How to actually implement the rule of five?

Question

UPDATE at the bottom

q1: How would you implement the rule of five for a class that manages rather heavy resources, but of which you want it to be passed around by value because that greatly simplifies and beautifies it's usage? Or are not all five items of the rule even needed?

In practice, I'm starting something with 3D imaging where an image is usually 128*128*128 doubles. Being able though to write things like this would make the math alot easier:

Data a = MakeData();
Data c = 5 * a + ( 1 + MakeMoreData() ) / 3;

q2: Using a combination of copy elision / RVO / move semantics the compiler should be able to this this with a minimum of copying, no?

I tried to figure out how to do this so I started with the basics; suppose an object implementing the traditional way of implementing copy and assignment:

class AnObject
{
public:
  AnObject( size_t n = 0 ) :
    n( n ),
    a( new int[ n ] )
  {}
  AnObject( const AnObject& rh ) :
    n( rh.n ),
    a( new int[ rh.n ] )
  {
    std::copy( rh.a, rh.a + n, a );
  }
  AnObject& operator = ( AnObject rh )
  {
    swap( *this, rh );
    return *this;
  }
  friend void swap( AnObject& first, AnObject& second )
  {
    std::swap( first.n, second.n );
    std::swap( first.a, second.a );
  }
  ~AnObject()
  {
    delete [] a;
  }
private:
  size_t n;
  int* a;
};

Now enter rvalues and move semantics. As far as I can tell this would be a working implementation:

AnObject( AnObject&& rh ) :
  n( rh.n ),
  a( rh.a )
{
  rh.n = 0;
  rh.a = nullptr;
}

AnObject& operator = ( AnObject&& rh )
{
  n = rh.n;
  a = rh.a;
  rh.n = 0;
  rh.a = nullptr;
  return *this;
}

However the compiler (VC++ 2010 SP1) is not too happy with this, and compilers are usually correct:

AnObject make()
{
  return AnObject();
}

int main()
{
  AnObject a;
  a = make(); //error C2593: 'operator =' is ambiguous
}

q3: How to solve this? Going back to AnObject& operator = ( const AnObject& rh ) certainly fixes it but don't we lose a rather important optimization opportunity?

Apart from that, it's clear that the code for the move constructor and assignment is full of duplication. So for now we forget about the ambiguity and try to solve this using copy and swap but now for rvalues. As explained here we wouldn't even need a custom swap but instead have std::swap do all the work, which sounds very promising. So I wrote the following, hoping std::swap would copy construct a temporary using the move constructor, then swap it with *this:

AnObject& operator = ( AnObject&& rh )
{
  std::swap( *this, rh );
  return *this;
}

But that doesn't work out and instead leads to a stack overflow due to infinite recursion since std::swap calls our operator = ( AnObject&& rh ) again. q4: Can someone provide an example of what is meant in the example then?

We can solve this by providing a second swap function:

AnObject( AnObject&& rh )
{
  swap( *this, std::move( rh ) );
}

AnObject& operator = ( AnObject&& rh )
{
  swap( *this, std::move( rh ) );
  return *this;
}

friend void swap( AnObject& first, AnObject&& second )
{
  first.n = second.n;
  first.a = second.a;
  second.n = 0;
  second.a = nullptr;
}

Now there's almost twice the amount code, however the move part of it pays of by alowing pretty cheap moving; but on the other hand the normal assignment can't benefit from copy elision anymore. At this point I'm really confused though, and not seeing anymore what's right and wrong, so I'm hoping to get some input here..

UPDATE So it seems there are two camps:

• one saying to skip the move assignment operator and continue doing what C++03 taught us, ie write a single assignment operator that passes the argument by value.
• the other one saying to implement the move assignment operator (after all, it's C++11 now) and have the copy assignment operator take its argument by reference.

(ok and there's the 3rd camp telling me to use a vector, but that's sort of out of scope for this hypothetical class. Ok in real life I would use a vector, and there would be also other members, but since the move constructor/assignment are not automatically generated (yet?) the question would still hold)

Unfortunately I cannot test both implementations in a real world scenario since this project has just started and the way the data will actually flow is not known yet. So I simply implemented both of them, added counters for allocation etc and ran a couple of iterations of approx. this code, where T is one of the implementations:

template< class T >
T make() { return T( narraySize ); }

template< class T >
void assign( T& r ) { r = make< T >(); }

template< class T >
void Test()
{
  T a;
  T b;
  for( size_t i = 0 ; i < numIter ; ++i )
  {
    assign( a );
    assign( b );
    T d( a );
    T e( b );
    T f( make< T >() );
    T g( make< T >() + make< T >() );
  }
}

Either this code is not good enough to test what I'm after, or the compiler is just too smart: doesn't matter what I use for arraySize and numIter, the results for both camps are pretty much identical: same number of allocations, very slight variations in timing but no reproducable significant difference.

So unless someone can point to a better way to test this (given that the actual usage scnearios are not known yet), I'll have to conclude that it doesn't matter and hence is left to the taste of the developper. In which case I'd pick #2.

Answer 1

You've missed a significant optimization in your copy assignment operator. And subsequently the situation has gotten confused.

  AnObject& operator = ( const AnObject& rh )
  {
    if (this != &rh)
    {
      if (n != rh.n)
      {
         delete [] a;
         n = 0;
         a = new int [ rh.n ];
         n = rh.n;
      }
      std::copy(rh.a, rh.a+n, a);
    }
    return *this;
  }

Unless you really never think you'll be assigning AnObjects of the same size, this is much better. Never throw away resources if you can recycle them.

Some might complain that the AnObject's copy assignment operator now has only basic exception safety instead of strong exception safety. However consider this:

Your clients can always take a fast assignment operator and give it strong exception safety. But they can't take a slow assignment operator and make it faster.

template <class T>
T&
strong_assign(T& x, T y)
{
    swap(x, y);
    return x;
}

Your move constructor is fine, but your move assignment operator has a memory leak. It should be:

  AnObject& operator = ( AnObject&& rh )
  {
    delete [] a;
    n = rh.n;
    a = rh.a;
    rh.n = 0;
    rh.a = nullptr;
    return *this;
  }

...

Data a = MakeData();
Data c = 5 * a + ( 1 + MakeMoreData() ) / 3;

q2: Using a combination of copy elision / RVO / move semantics the compiler should be able to this this with a minimum of copying, no?

You may need to overload your operators to take advantage of resources in rvalues:

Data operator+(Data&& x, const Data& y)
{
   // recycle resources in x!
   x += y;
   return std::move(x);
}

Ultimately resources ought to be created exactly once for each Data you care about. There should be no needless new/delete just for the purpose of moving things around.

Answer 2

If your object is resource-heavy, you might want to avoid copying altogether, and just provide the move constructor and move assignment operator. However, if you really want copying too, it is easy to provide all of the operations.

Your copy operations look sensible, but your move operations don't. Firstly, though an rvalue reference parameter will bind to an rvalue, within the function it is an lvalue, so your move constructor ought to be:

AnObject( AnObject&& rh ) :
  n( std::move(rh.n) ),
  a( std::move(rh.a) )
{
  rh.n = 0;
  rh.a = nullptr;
}

Of course, for fundamental types like you've got here it doesn't actually make a difference, but it's as well to get in the habit.

If you provide a move-constructor, then you don't need a move-assignment operator when you define copy-assignment like you have --- because you accept the parameter by value, an rvalue will be moved into the parameter rather than copied.

As you found, you can't use std::swap() on the whole object inside a move-assignment operator, since that will recurse back into the move-assignment operator. The point of the comment in the post you linked to is that you don't need to implement a custom swap if you provide move operations, as std::swap will use your move operations. Unfortunately, if you don't define a separate move assignment operator this doesn't work, and will still recurse. You can of course use std::swap to swap the members:

AnObject& operator=(AnObject other)
{
    std::swap(n,other.n);
    std::swap(a,other.a);
    return *this;
}

Your final class is thus:

class AnObject
{
public:
  AnObject( size_t n = 0 ) :
    n( n ),
    a( new int[ n ] )
  {}
  AnObject( const AnObject& rh ) :
    n( rh.n ),
    a( new int[ rh.n ] )
  {
    std::copy( rh.a, rh.a + n, a );
  }
  AnObject( AnObject&& rh ) :
    n( std::move(rh.n) ),
    a( std::move(rh.a) )
  {
    rh.n = 0;
    rh.a = nullptr;
  }
  AnObject& operator = ( AnObject rh )
  {
    std::swap(n,rh.n);
    std::swap(a,rh.a);
    return *this;
  }
  ~AnObject()
  {
    delete [] a;
  }
private:
  size_t n;
  int* a;
};

Answer 3

Let me help you:

#include <vector>

class AnObject
{
public:
  AnObject( size_t n = 0 ) : data(n) {}

private:
  std::vector<int> data;
};

From the C++0x FDIS, [class.copy] note 9:

If the definition of a class X does not explicitly declare a move constructor, one will be implicitly declared as defaulted if and only if

• X does not have a user-declared copy constructor,

• X does not have a user-declared copy assignment operator,

• X does not have a user-declared move assignment operator,

• X does not have a user-declared destructor, and

• the move constructor would not be implicitly defined as deleted.

[ Note: When the move constructor is not implicitly declared or explicitly supplied, expressions that otherwise would have invoked the move constructor may instead invoke a copy constructor. —end note ]

Personally, I am much more confident in std::vector correctly managing its resources and optimizing the copies / moves that in any code I could write.

Answer 4

Since I haven't seen anyone else explicitly point this out...

Your copy assignment operator taking its argument by value is an important optimization opportunity if (and only if) it's passed an rvalue, due to copy elision. But in a class with an assignment operator that explicitly only takes rvalues (i.e., one with a move assignment operator), this is a nonsensical scenario. So, modulo the memory leaks that have already been pointed out in other answers, I'd say your class is already ideal if you simply change the copy assignment operator to take its argument by const reference.

Answer 5

q3 of the Original Poster

I think you (and some of the other responders) misunderstood what the compiler error meant, and came to the wrong conclusions because of it. The compiler thinks that the (move) assignment call is ambiguous, and it's right! You have multiple methods that are equally qualified.

In your original version of the AnObject class, your copy constructor takes in the old object by const (lvalue) reference, while the assignment operator takes its argument by (unqualified) value. The value argument is initialized by the appropriate transfer constructor from whatever was on the right-side of the operator. Since you have only one transfer constructor, that copy constructor is always used, no matter if the original right-side expression was a lvalue or a rvalue. This makes the assignment operator act as the copy-assignment special member function.

The situation changes once a move constructor is added. Whenever the assignment operator is called, there are two choices for the transfer constructor. The copy constructor will still be used for lvalue expressions, but the move constructor will be used whenever a rvalue expression is given instead! This makes the assignment operator simultaneously act as the move-assignment special member function.

When you added a traditional move-assignment operator, you gave the class two versions of the same special member function, which is an error. You already have what you wanted, so just get rid of the traditional move-assignment operator, and no other changes should be needed.

In the two camps listed in your update, I guess I'm technically in the first camp, but for entirely different reasons. (Don't skip the (traditional) move-assignment operator because it's "broken" for your class, but because it's superfluous.)

BTW, I'm new to reading about C++11 and StackOverflow. I came up with this answer from browsing another S.O. question before seeing this one. (Update: Actually, I still had the page open. The link goes to the specific response by FredOverflow that shows the technique.)

About the 2011-May-12 Response by Howard Hinnant

(I'm too much of a newbie to directly comment on responses.)

You don't need to explicitly check for self-assignment if a later test would already cull it. In this case, n != rh.n would already take care of most of it. However, the std::copy call is outside of that (currently) inner if, so we would get n component-level self-assignments. It's up to you to decide if those assignments would be too anti-optimal even though self-assignment should be rare.

Answer 6

With help of delegating constructor, you only need to implement each concept once;

• default init
• resource delete
• swap
• copy

the rest just uses those.

Also don't forget to make move-assignment (and swap) noexcept, if helps performance a lot if you, for example, put your class in a vector

#include <utility>

// header

class T
{
public:
    T();
    T(const T&);
    T(T&&);
    T& operator=(const T&);
    T& operator=(T&&) noexcept;
    ~T();

    void swap(T&) noexcept;

private:
    void copy_from(const T&);
};

// implementation

T::T()
{
    // here (and only here) you implement default init
}

T::~T()
{
    // here (and only here) you implement resource delete
}

void T::swap(T&) noexcept
{
    using std::swap; // enable ADL
    // here (and only here) you implement swap, typically memberwise swap
}

void T::copy_from(const T& t)
{
    if( this == &t ) return; // don't forget to protect against self assign
    // here (and only here) you implement copy
}

// the rest is generic:

T::T(const T& t)
    : T()
{
    copy_from(t);
}

T::T(T&& t)
    : T()
{
    swap(t);
}

auto T::operator=(const T& t) -> T&
{
    copy_from(t);
    return *this;
}

auto T::operator=(T&& t) noexcept -> T&
{
    swap(t);
    return *this;
}