Trying to Write Move Constructor in terms of Move Assignment

Question

So playing around with Move Semantics.

So my first look at this went like this:

 class String
 {
     char*   data;
     int     len;
     public:
         // Normal rule of three applied up here.
         void swap(String& rhs) throw()
         {
            std::swap(data, rhs.data);
            std::swap(len,  rhs.len);
         }
         String& operator=(String rhs) // Standard Copy and swap. 
         {
            rhs.swap(*this);
            return *this;
         }

         // New Stuff here.
         // Move constructor
         String(String&& cpy) throw()    // ignore old throw construct for now.  
            : data(NULL)
            , len(0)
         {
            cpy.swap(*this);
         }
         String& operator=(String&& rhs) throw() 
         {
            rhs.swap(*this);
            return *this;
         }
};

Looking at this. I though it may be worth defining the Move constructor in terms of Move assignment. It has a nice symmetry to it and I like it because it also looks DRY (and like copy and swap).

So I re-wrote the Move Constructor as:

         String(String&& cpy) throw() 
            : data(NULL)
            , len(0)
         {
            operator=(std::move(cpy));
         }

But this generates an ambiguity error:

String.cpp:45:9: error: call to member function 'operator=' is ambiguous
        operator=(std::move(rhs));
        ^~~~~~~~~
String.cpp:32:13: note: candidate function
    String& operator=(String rhs)
            ^
String.cpp:49:13: note: candidate function
    String& operator=(String&& rhs) throw()
            ^
1 error generated.

Since I used std::move() while passing the argument I was expecting this to bind to the Move assignment operator. What am I doing wrong?

Answer 1

What am I doing wrong?

It should be a rare occurrence that you try to write one special member function in terms of another. Each special member typically needs special attention. If after the exercise of making each special member as efficient as possible, you see an opportunity to consolidate code, then, and only then, go to the effort.

Starting with the goal of consolidating code among the special members is the wrong place to start.

Step 1. Start with trying to write your special members with = default.

Step 2. When that fails, then customize each one that can not be written with = default.

Step 3. Write tests to confirm that Step 2 is working.

Step 4. Once Step 3 is done, see if there are code consolidations you can make without sacrificing performance. This may involve writing performance tests.

Jumping straight to Step 4 is error prone, and often leads to significant performance penalties.

Here is Step 2 for your example:

#include <algorithm>

 class String
 {
     char*   data;
     int     len;
     public:
         String() noexcept
            : data(nullptr)
            , len(0)
            {}

         ~String()
         {
            delete [] data;
         }

         String(const String& cpy)
            : data(new char [cpy.len])
            , len(cpy.len)
         {
            std::copy(cpy.data, cpy.data+cpy.len, data);
         }

         String(String&& cpy) noexcept
            : data(cpy.data)
            , len(cpy.len)
         {
            cpy.data = nullptr;
            cpy.len = 0;
         }

         String& operator=(const String& rhs)
         {
            if (this != &rhs)
            {
                if (len != rhs.len)
                {
                    delete [] data;
                    data = nullptr;
                    len = 0;
                    data = new char[rhs.len];
                    len = rhs.len;
                }
                std::copy(rhs.data, rhs.data+rhs.len, data);
            }
            return *this;
         }

         String& operator=(String&& rhs) noexcept
         {
            delete [] data;
            data = nullptr;
            len = 0;
            data = rhs.data;
            len = rhs.len;
            rhs.data = nullptr;
            rhs.len = 0;
            return *this;
         }

         void swap(String& rhs) noexcept
         {
            std::swap(data, rhs.data);
            std::swap(len,  rhs.len);
         }
};

Update

It should be noted that in C++98/03 one can not successfully overload functions whose parameters differ only between by-value and by-reference. For example:

void f(int);
void f(int&);

int
main()
{
    int i = 0;
    f(i);
}

test.cpp:8:5: error: call to 'f' is ambiguous
    f(i);
    ^
test.cpp:1:6: note: candidate function
void f(int);
     ^
test.cpp:2:6: note: candidate function
void f(int&);
     ^
1 error generated.

Adding const doesn't help:

void f(int);
void f(const int&);

int
main()
{
    f(0);
}

test.cpp:7:5: error: call to 'f' is ambiguous
    f(0);
    ^
test.cpp:1:6: note: candidate function
void f(int);
     ^
test.cpp:2:6: note: candidate function
void f(const int&);
     ^
1 error generated.

These same rules apply to C++11, and are extended without modification to rvalue-references:

void f(int);
void f(int&&);

int
main()
{
    f(0);
}

test.cpp:7:5: error: call to 'f' is ambiguous
    f(0);
    ^
test.cpp:1:6: note: candidate function
void f(int);
     ^
test.cpp:2:6: note: candidate function
void f(int&&);
     ^
1 error generated.

And so it is unsurprising that given:

String& operator=(String rhs);
String& operator=(String&& rhs) throw();

the result is:

String.cpp:45:9: error: call to member function 'operator=' is ambiguous
        operator=(std::move(rhs));
        ^~~~~~~~~
String.cpp:32:13: note: candidate function
    String& operator=(String rhs)
            ^
String.cpp:49:13: note: candidate function
    String& operator=(String&& rhs) throw()
            ^
1 error generated.

Answer 2

I believe the copy constructor will have to be written:

     String& operator=(const String &rhs_ref) // (not-so-standard) Copy and Swap. 
     {
        String rhs(rhs_ref); // This is the copy
        rhs.swap(*this);     // This is the swap
        return *this;
     }

In C++03, the objection to this approach would be that it's difficult for the compiler to fully optimize this. In C++03, it's nice to use operator=(String rhs) as there are situations where the compiler can skip the copy step and build the parameter in place. For example, even in C++03, a call to String s; s = func_that_returns_String_by_value(); can be optimized to skip the copy.

So "copy and swap" should be renamed to "copy only if necessary, then perform a swap".

The compiler (in C++03 or C++11), takes one of two routes:

1. a (necessary) copy, followed by a swap
2. no copy, just do a swap

We can write operator=(String rhs) as the optimal way to handle both situations.

But that objection doesn't apply when a move-assignment operator is present. In situations where the copy can be skipped, the operator=(String && rhs) will take over. This takes care of the second situation. Therefore, we need only implement the first situation, and we use String(const String &rhs_ref) to do that.

It has the disadvantage of requiring a little more typing as we have to do the copy more explicitly, but I'm not aware of any optimization opportunity that is missing here. (But I'm no expert ...)

Answer 3

I'll put this as an answer so I can attempt to write readable code to discuss, but my semantics may be mixed up as well (so consider it a work in process):

std::move returns an xvalue, but you really want an rvalue, so it would seem to me that this should work instead:

String(String&& cpy) throw() : data(NULL), len(0)
{
    operator=(std::forward<String>(cpy));
    //        ^^^^^^^^^^^^ returns an rvalue 
}

Since std::forward will give you an rvalue, and operator=(String&&) is expecting one. It would seem to me that would make sense to use instead of std::move.

EDIT

I did a little experiment (http://ideone.com/g0y3PL). It appears that the compiler cannot tell the difference between String& operator=(String) and String& operator=(String&&); however, if you change the copy-assignment operator's signature to String& operator=(const String&), it is no longer ambiguous.

I'm not sure if that is a bug in the compiler or something I'm missing in the standard somewhere, but it would seem like it should be able to tell the difference between a copy and an rvalue reference.

In conclusion, it appears that Howard's note about not implementing special functions in terms of other special functions would be a better way to go.

Answer 4

(Sorry for adding a third answer, but I think I've finally got a solution I'm satisfied with. Demo on ideone)

You have a class with both of these methods:

String& operator=(String copy_and_swap);
String& operator=(String && move_assignment);

The problem is ambiguity. We want a tie-breaker in favour of the second option as the second overload can, where it's viable, be more efficient. Therefore, we replace the first version with a templated method:

template<typename T>
String& operator=(T templated_copy_and_swap);
String& operator=(String && move_assignment);

This tiebreaks in favour of the latter, as desired, but unfortunately we get an error message: error: object of type 'String' cannot be assigned because its copy assignment operator is implicitly deleted.

But we can fix this. We need to declare a copy assignment operator, so that it doesn't decide to implicitly delete it, but we must also ensure we don't introduce any more ambiguity. Here's one way to do it.

const volatile String&& operator=(String&) volatile const && = delete;

Now we have three assignment operators (one of which is deleted) with the appropriate tie-breaking. Note the volatile const &&. The purpose of this is to simply add as many qualifiers as possible in order to give this overload very low precedence. And, in the unlikely event that you do try to assign to an object that is volatile const &&, then you'll get a compiler error and you can deal with it then.

(Tested with clang 3.3 and g++-4.6.3, and it does the desired number of copies and swaps (i.e. as few as possible! With g++, you need volatile const instead of volatile const && but that's OK.)

Edit: Type deduction risk: In the implementation of the templated operator=, you might want to consider being careful about the deduced type, something like static_assert( std::is_same<T,String>(), "This should only accept Strings. Maybe SFINAE and enable_if on the return value?");.