Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am trying to compose an illustrative example which shows how to implement move semantics on an object that will be stored in a vector.

Please consider the following code, which is my illustrative example so far. It is designed to be a canonical, pedantically correct implementation of an object that implements move semantics, does not implement copy semantics, and can be stored in a vector<> (Where T is Moveable, below). How did I do?

#include <string>
#include <iostream>
#include <vector>
#include <list>
#include <algorithm>
#include <memory>
#include <iterator>
using namespace std;

class Moveable
{
public:
    string foo_;
    string bar_;

    Moveable(Moveable&& rhs) : foo_(std::move(rhs.foo_)), bar_(std::move(rhs.bar_)) {}  // move construction
    Moveable(const string& foo) : foo_(foo) {};             // convert construction
    Moveable& operator=(Moveable&& rhs)     // move assignment
    {
        foo_ = std::move(rhs.foo_);
        bar_ = std::move(rhs.bar_);
        return * this;
    }
private:
    Moveable(const Moveable&);              // not defined, not copy-constructible
    Moveable& operator=(const Moveable&);   // not defined, not copy-assignable
    Moveable();                             // not defined, not default constructible
};

Moveable generate_it()
{
    static string foo ;
    if( foo.empty() || foo[0] == 'z' )
        foo.insert(0, 1, 'a');
    else
        foo[0]++;
    return foo;
}

int main()
{
    typedef vector<Moveable> Moveables;
    Moveables v;
    generate_n(back_inserter(v), 1024, &generate_it);
    cout << v.size() << endl;
    string target = "zzz";
    auto that = find_if(v.begin(), v.end(), [target](const Moveables::value_type& it) -> bool
    {
        return it.foo_ == target;
    });

    if( that != v.end() )
        v.erase(that);

    sort(v.begin(), v.end(), [](const Moveables::value_type& lhs, const Moveables::value_type& rhs) -> bool
    {
        return lhs.foo_ > rhs.foo_;
    });
    cout << v.size() << endl;
}
share|improve this question
    
I had posted another question very similar to this, but deleted it and replaced it with this one. The other question had used unique_ptr which ultimately was orthogonal to what I was really going for. –  John Dibling Jun 22 '12 at 17:18
add comment

1 Answer 1

up vote 6 down vote accepted
  • One small error: Mark the move constructor as noexcept, otherwise there are situations where it won’t be used. The linked case is different since the class has a copy-constructor, yet I can imagine that there are still situations where it matters, especially since your copycon isn’t deleted, just private and undefined.

  • Why is the destructor virtual? If the example is supposed to be minimal then this might be distracting.

  • In generate_it:

    return std::move(Moveable(foo));
    

    The std::move here is redundant, since you are returning a temporary. What’s more, the explicit constructor call is redundant too, since the constructor isn’t marked as explicit. Just return foo; will do.

  • The find_if call could be replaced by vanilla find, using temporary construction again:

    auto that = find(v.begin(), v.end(), target);
    
  • Finally, in the sort call, why aren’t the arguments declared const&? Granted, makes the line even longer as it stands this is inconsistent with the const-correctness illustrated in the find_if call.

share|improve this answer
    
+1: various replies. 1) The destructor was an artifact of my testing durig development. Since its implementation was trivial, and the tor was public, I've removed it entirely. –  John Dibling Jun 26 '12 at 16:35
    
2) noexcept is not supported by MSVC10. Sad. –  John Dibling Jun 26 '12 at 16:38
    
3) Fixed generate_it 4) fixed sort call. These were simply part of the harness, but it's important that the whole thing is correct. –  John Dibling Jun 26 '12 at 16:40
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.