Move Assignment incompatible with Standard Copy and Swap

Question

Testing out the new Move Semantics.

I just asked about an issues I was having with the Move Constructor. But as it turns out in the comments the problem is really that the "Move Assignment" operator and "Standard Assignment" operator clash when you use the standard "Copy and Swap" idiom.

This is the class I am using:

#include <string.h>
#include <utility>

class String
{
    int         len;
    char*       data;

    public:
        // Default constructor
        // In Terms of C-String constructor
        String()
            : String("")
        {}

        // Normal constructor that takes a C-String
        String(char const* cString)
            : len(strlen(cString))
            , data(new char[len+1]()) // Allocate and zero memory
        {
            memcpy(data, cString, len);
        }

        // Standard Rule of three
        String(String const& cpy)
            : len(cpy.len)
            , data(new char[len+1]())
        {
            memcpy(data, cpy.data, len);
        }
        String& operator=(String rhs)
        {
            rhs.swap(*this);
            return *this;
        }
        ~String()
        {
            delete [] data;
        }
        // Standard Swap to facilitate rule of three
        void swap(String& other) throw ()
        {
            std::swap(len,  other.len);
            std::swap(data, other.data);
        }

        // New Stuff
        // Move Operators
        String(String&& rhs) throw()
            : len(0)
            , data(null)
        {
            rhs.swap(*this);
        }
        String& operator=(String&& rhs) throw()
        {
            rhs.swap(*this);
            return *this;
        }
};

Pretty bog standard I think.

Then I tested my code like this:

int main()
{
    String  a("Hi");
    a   = String("Test Move Assignment");
}

Here the assignment to a should use the "Move Assignment" operator. But there is a clash with the "Standard Assignment" operator (which is written as your standard copy and swap).

> g++ --version
Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.9.sdk/usr/include/c++/4.2.1
Apple LLVM version 5.0 (clang-500.2.79) (based on LLVM 3.3svn)
Target: x86_64-apple-darwin13.0.0
Thread model: posix

> g++ -std=c++11 String.cpp
String.cpp:64:9: error: use of overloaded operator '=' is ambiguous (with operand types 'String' and 'String')
    a   = String("Test Move Assignment");
    ~   ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
String.cpp:32:17: note: candidate function
        String& operator=(String rhs)
                ^
String.cpp:54:17: note: candidate function
        String& operator=(String&& rhs)
                ^

Now I can fix this by modifying the "Standard Assignment" operator to:

    String& operator=(String const& rhs)
    {
        String copy(rhs);
        copy.swap(*this);
        return *this;
    }

But this is not good as it messes with the compiler's ability to optimize the copy and swap. See What is the copy-and-swap idiom? here and here

Am I missing something not so obvious?

Anthropophagi

It sounds like one of those Apple "pretend" C++11 libraries. It does not offer the feature. Its caused me so much grief in the past I can't recount all the problems. Guard the code path with __has_feature(cxx_implicit_moves). Another one is STLport on Android. – Rue

Answer 1

If you define the assignment operator to take a value, you should not (need not and cannot) define the assignment operator taking an rvalue-reference. There is no point to it.

In general, you only need to provide an overload taking an rvalue-reference when you need to differentiate an lvalue from an rvalue, but in this case your choice of implementation means that you don't need to make that distinction. Whether you have an lvalue or an rvalue you are going to create the argument and swap the contents.

String f();
String a;
a = f();   // with String& operator=(String)

In this case, the compiler will resolve the call to be a.operator=(f()); it will realize that the only reason for the return value is being the argument to operator= and will elide any copy --this is the point of making the function take a value in the first place!

Prieto

If you have just operator =(String) you lose a performance optimization when assigning from an lvalue whose len is smaller than that of target. See my post. – Groves

@CassioNeri: Nice optimization. But I think that it is specific to this type of problem (my fault for picking this as an example). I am trying to look at the general case. – Anthropophagi

@CassioNeri: For that to work, there would have to be two separate members for the length and the capacity. Even with that change, and n the general case, say for a handcrafted implementation of vector, iwhere the copies might throw you would still need to make the copy aside if you want to provide the strong exception guarantee. – Splitlevel

@CassioNeri: Other than that, you are right, if you want different behavior for lvalues and rvalues then you overload, but you overload on lvalue-reference and rvalue-reference, not on value vs. any of the reference types – Splitlevel

So short: You are saying the if you have a "Standard Assignment" (done with copy and swap) the compiler optimizations (RVO) work well enough that "Move Assignment" is not required. – Anthropophagi

@LokiAstari: Correct, but Cassio is right that if you really care about performance, you might be able to do better by handcrafting the solution to the exact problem. That is taking the argument to operator= by value is a good general start point, in some particular cases you can do better by providing different implementations for the lvalue/rvalue cases, but that is on a per type base – Splitlevel

Thanks. Just trying to work this all out (I have access to C+11 at work now) :-) – Anthropophagi

String f(); //this looks like a function declaration? – Hagiolatry

@StereoMatching: That is a function declaration – Splitlevel

I profiled this recently and providing the separate rvalue reference ctor and assign as well as lvalue reference ctor and assign is faster in almost all cases. The only case this doesn't hold up is the copy assignment passing by lvalue reference when you're having to allocate resources (larger buffer for instance). Even then its only very slightly slower than pass by value. The difference is that when passing by value in place of rvalue reference, you're always getting to move ctor calls instead of just one. – Kawasaki

@Troy: Is the profiling program and data available? I'd like to take a look at it. – Splitlevel

Answer 2

Other answers suggest to have just one overload operator =(String rhs) taking the argument by value but this is not the most efficient implementation.

It's true that in this example by David Rodríguez - dribeas

String f();
String a;
a = f();   // with String& operator=(String)

no copy is made. However, assume just operator =(String rhs) is provided and consider this example:

String a("Hello"), b("World");
a = b;

What happens is

1. b is copied to rhs (memory allocation + memcpy);
2. a and rhs are swapped;
3. rhs is destroyed.

If we implement operator =(const String& rhs) and operator =(String&& rhs) then we can avoid the memory allocation in step 1 when the target has a length bigger than source's. For instance, this is a simple implementation (not perfect: could be better if String had a capacity member):

String& operator=(const String& rhs) {
    if (len < rhs.len) {
        String tmp(rhs);
        swap(tmp);
    else {
        len = rhs.len;
        memcpy(data, rhs.data, len);
        data[len] = 0;
    }
    return *this;
}

String& operator =(String&& rhs) {
    swap(rhs);
}

In addition to the performance point if swap is noexcept, then operator =(String&&) can be noexcept as well. (Which is not the case if memory allocation is "potentially" performed.)

See more details in this excellent explanation by Howard Hinnant.

Groves

Would it be fair to say then, that the copy-assignment operator should take its argument by value if and only if you do not wish to write a separate move-assignment operator? – Arkhangelsk

@MattMcNabb IMHO this is a very reasonable guideline but, as it happens sometimes, there might cases where it shouldn't apply (I don't have any example to offer). – Groves

You need to be careful in general however with "operator=(Whatever &&rhs)". "rhs" will still be alive when the function exits (its destructor hasn't run yet), so the resources formerly stored in the target object aren't free yet (surprise!). If someone calls this via "obj1 = std::move(obj2)", and "Whatever" holds some type of resource that affects the outside world (an exclusive handle, lock, huge chunk of memory, etc.), then you should explicitly free the resource before exiting the function. C++ is full of trip wires and I still struggle with it myself after 30+ years (in C and then C++). – Eisenberg

@Eisenberg that makes no sense. If the lock is exclusive and at the end of the function both objects hold it, then you're doing something insane. If the rvalue still owns a large chunk of memory, why would you need to free it? It's destructor is going to do that anyway. It sounds like you are writing your move operators as though they are copy operators. – Chopfallen

@kfsone: Both objects don't hold it because they've simply exchanged resources. It's beside the point though. The issue is that the resources formerly held by the target object are still alive inside of "rhs" when the assignment is over (i.e., not yet destroyed). So in the words of Peter Beckley, you've " ... drifted into the netherworld of non-deterministic destruction". See his article here thbecker.net/articles/rvalue_references/section_01.html (the situation is buried somewhere in the article but it's a widely known issue). – Eisenberg

Answer 3

All you need for copy and assignment is this:

    // As before
    String(const String& rhs);

    String(String&& rhs)
    :   len(0), data(0)
    {
        rhs.swap(*this);
    }

    String& operator = (String rhs)
    {
        rhs.swap(*this);
        return *this;
    }

   void swap(String& other) noexcept {
       // As before
   }

Aftersensation

@Raxvan Interesting but not the best implementation for this class according to this excellent explanation by Howard Hinnant. – Groves

@CassioNeri Almost a good point if the String has a notion of capacity – Aftersensation

@DieterLücking True that the best is achieved if String had a capacity member. However, even without it (just using len) we achieve better performance when the source is an lvalue whose len is smaller than that of target. See my post. – Groves

@CassioNeri Hence a matter of design decision is left: Is it better to copy the content and keep an (possible) unused extra capacity or assign content and capacity (which may be extra unused capacity, too) or an assignment of an iterator range assign(first last)) ? – Aftersensation

Why 'rhs.swap(*this)' instead of 'swap(rhs)'? godbolt.org/g/v3M5xy – Chopfallen