Move assignment operator and `if (this != &rhs)`

Question

Wow, there is just so much to clean up here...

First, the Copy and Swap is not always the correct way to implement Copy Assignment. Almost certainly in the case of dumb_array, this is a sub-optimal solution.

The use of Copy and Swap is for dumb_array is a classic example of putting the most expensive operation with the fullest features at the bottom layer. It is perfect for clients who want the fullest feature and are willing to pay the performance penalty. They get exactly what they want.

But it is disastrous for clients who do not need the fullest feature and are instead looking for the highest performance. For them dumb_array is just another piece of software they have to rewrite because it is too slow. Had dumb_array been designed differently, it could have satisfied both clients with no compromises to either client.

The key to satisfying both clients is to build the fastest operations in at the lowest level, and then to add API on top of that for fuller features at more expense. I.e. you need the strong exception guarantee, fine, you pay for it. You don't need it? Here's a faster solution.

Let's get concrete: Here's the fast, basic exception guarantee Copy Assignment operator for dumb_array:

dumb_array& operator=(const dumb_array& other)
{
    if (this != &other)
    {
        if (mSize != other.mSize)
        {
            delete [] mArray;
            mArray = nullptr;
            mArray = other.mSize ? new int[other.mSize] : nullptr;
            mSize = other.mSize;
        }
        std::copy(other.mArray, other.mArray + mSize, mArray);
    }
    return *this;
}

Explanation:

One of the more expensive things you can do on modern hardware is make a trip to the heap. Anything you can do to avoid a trip to the heap is time & effort well spent. Clients of dumb_array may well want to often assign arrays of the same size. And when they do, all you need to do is a memcpy (hidden under std::copy). You don't want to allocate a new array of the same size and then deallocate the old one of the same size!

Now for your clients who actually want strong exception safety:

template <class C>
C&
strong_assign(C& lhs, C rhs)
{
    swap(lhs, rhs);
    return lhs;
}

Or maybe if you want to take advantage of move assignment in C++11 that should be:

template <class C>
C&
strong_assign(C& lhs, C rhs)
{
    lhs = std::move(rhs);
    return lhs;
}

If dumb_array's clients value speed, they should call the operator=. If they need strong exception safety, there are generic algorithms they can call that will work on a wide variety of objects and need only be implemented once.

Now back to the original question (which has a type-o at this point in time):

Class&
Class::operator=(Class&& rhs)
{
    if (this == &rhs)  // is this check needed?
    {
       // ...
    }
    return *this;
}

This is actually a controversial question. Some will say yes, absolutely, some will say no.

My personal opinion is no, you don't need this check.

Rationale:

When an object binds to an rvalue reference it is one of two things:

1. A temporary.
2. An object the caller wants you to believe is a temporary.

If you have a reference to an object that is an actual temporary, then by definition, you have a unique reference to that object. It can't possibly be referenced by anywhere else in your entire program. I.e. this == &temporary is not possible.

Now if your client has lied to you and promised you that you're getting a temporary when you're not, then it is the client's responsibility to be sure that you don't have to care. If you want to be really careful, I believe that this would be a better implementation:

Class&
Class::operator=(Class&& other)
{
    assert(this != &other);
    // ...
    return *this;
}

I.e. If you are passed a self reference, this is a bug on the part of the client that should be fixed.

For completeness, here is a move assignment operator for dumb_array:

dumb_array& operator=(dumb_array&& other)
{
    assert(this != &other);
    delete [] mArray;
    mSize = other.mSize;
    mArray = other.mArray;
    other.mSize = 0;
    other.mArray = nullptr;
    return *this;
}

In the typical use case of move assignment, *this will be a moved-from object and so delete [] mArray; should be a no-op. It is critical that implementations make delete on a nullptr as fast as possible.

Caveat:

Some will argue that swap(x, x) is a good idea, or just a necessary evil. And this, if the swap goes to the default swap, can cause a self-move-assignment.

I disagree that swap(x, x) is ever a good idea. If found in my own code, I will consider it a performance bug and fix it. But in case you want to allow it, realize that swap(x, x) only does self-move-assignemnet on a moved-from value. And in our dumb_array example this will be perfectly harmless if we simply omit the assert, or constrain it to the moved-from case:

dumb_array& operator=(dumb_array&& other)
{
    assert(this != &other || mSize == 0);
    delete [] mArray;
    mSize = other.mSize;
    mArray = other.mArray;
    other.mSize = 0;
    other.mArray = nullptr;
    return *this;
}

If you self-assign two moved-from (empty) dumb_array's, you don't do anything incorrect aside from inserting useless instructions into your program. This same observation can be made for the vast majority of objects.

<Update>

I've given this issue some more thought, and changed my position somewhat. I now believe that assignment should be tolerant of self assignment, but that the post conditions on copy assignment and move assignment are different:

For copy assignment:

x = y;

one should have a post-condition that the value of y should not be altered. When &x == &y then this postcondition translates into: self copy assignment should have no impact on the value of x.

For move assignment:

x = std::move(y);

one should have a post-condition that y has a valid but unspecified state. When &x == &y then this postcondition translates into: x has a valid but unspecified state. I.e. self move assignment does not have to be a no-op. But it should not crash. This post-condition is consistent with allowing swap(x, x) to just work:

template <class T>
void
swap(T& x, T& y)
{
    // assume &x == &y
    T tmp(std::move(x));
    // x and y now have a valid but unspecified state
    x = std::move(y);
    // x and y still have a valid but unspecified state
    y = std::move(tmp);
    // x and y have the value of tmp, which is the value they had on entry
}

The above works, as long as x = std::move(x) doesn't crash. It can leave x in any valid but unspecified state.

I see three ways to program the move assignment operator for dumb_array to achieve this:

dumb_array& operator=(dumb_array&& other)
{
    delete [] mArray;
    // set *this to a valid state before continuing
    mSize = 0;
    mArray = nullptr;
    // *this is now in a valid state, continue with move assignment
    mSize = other.mSize;
    mArray = other.mArray;
    other.mSize = 0;
    other.mArray = nullptr;
    return *this;
}

The above implementation tolerates self assignment, but *this and other end up being a zero-sized array after the self-move assignment, no matter what the original value of *this is. This is fine.

dumb_array& operator=(dumb_array&& other)
{
    if (this != &other)
    {
        delete [] mArray;
        mSize = other.mSize;
        mArray = other.mArray;
        other.mSize = 0;
        other.mArray = nullptr;
    }
    return *this;
}

The above implementation tolerates self assignment the same way the copy assignment operator does, by making it a no-op. This is also fine.

dumb_array& operator=(dumb_array&& other)
{
    swap(other);
    return *this;
}

The above is ok only if dumb_array does not hold resources that should be destructed "immediately". For example if the only resource is memory, the above is fine. If dumb_array could possibly hold mutex locks or the open state of files, the client could reasonably expect those resources on the lhs of the move assignment to be immediately released and therefore this implementation could be problematic.

The cost of the first is two extra stores. The cost of the second is a test-and-branch. Both work. Both meet all of the requirements of Table 22 MoveAssignable requirements in the C++11 standard. The third also works modulo the non-memory-resource-concern.

All three implementations can have different costs depending on the hardware: How expensive is a branch? Are there lots of registers or very few?

The take-away is that self-move-assignment, unlike self-copy-assignment, does not have to preserve the current value.

</Update>

One final (hopefully) edit inspired by Luc Danton's comment:

If you're writing a high level class that doesn't directly manage memory (but may have bases or members that do), then the best implementation of move assignment is often:

Class& operator=(Class&&) = default;

This will move assign each base and each member in turn, and will not include a this != &other check. This will give you the very highest performance and basic exception safety assuming no invariants need to be maintained among your bases and members. For your clients demanding strong exception safety, point them towards strong_assign.

First, you got the signature of the move-assignment operator wrong. Since moves steal resources from the source object, the source has to be a non-const r-value reference.

Class &Class::operator=( Class &&rhs ) {
    //...
    return *this;
}

Note that you still return via a (non-const) l-value reference.

For either type of direct assignment, the standard isn't to check for self-assignment, but to make sure a self-assignment doesn't cause a crash-and-burn. Generally, no one explicitly does x = x or y = std::move(y) calls, but aliasing, especially through multiple functions, may lead a = b or c = std::move(d) into being self-assignments. An explicit check for self-assignment, i.e. this == &rhs, that skips the meat of the function when true is one way to ensure self-assignment safety. But it's one of the worst ways, since it optimizes a (hopefully) rare case while it's an anti-optimization for the more common case (due to branching and possibly cache misses).

Now when (at least) one of the operands is a directly temporary object, you can never have a self-assignment scenario. Some people advocate assuming that case and optimize the code for it so much that the code becomes suicidally stupid when the assumption is wrong. I say that dumping the same-object check on users is irresponsible. We don't make that argument for copy-assignment; why reverse the position for move-assignment?

Let's make an example, altered from another respondent:

dumb_array& dumb_array::operator=(const dumb_array& other)
{
    if (mSize != other.mSize)
    {
        delete [] mArray;
        mArray = nullptr;  // clear this...
        mSize = 0u;        // ...and this in case the next line throws
        mArray = other.mSize ? new int[other.mSize] : nullptr;
        mSize = other.mSize;
    }
    std::copy(other.mArray, other.mArray + mSize, mArray);
    return *this;
}

This copy-assignment handles self-assignment gracefully without an explicit check. If the source and destination sizes differ, then deallocation and reallocation precede the copying. Otherwise, just the copying is done. Self-assignment does not get an optimized path, it gets dumped into the same path as when the source and destination sizes start off equal. The copying is technically unnecessary when the two objects are equivalent (including when they're the same object), but that's the price when not doing an equality check (value-wise or address-wise) since said check itself would be a waste most of the time. Note that the object self-assignment here will cause a series of element-level self-assignments; the element type has to be safe for doing this.

Like its source example, this copy-assignment provides the basic exception safety guarantee. If you want the strong guarantee, then use the unified-assignment operator from the original Copy and Swap query, which handles both copy- and move-assignment. But the point of this example is to reduce safety by one rank to gain speed. (BTW, we're assuming that the individual elements' values are independent; that there's no invariant constraint limiting some values compared to others.)

Let's look at a move-assignment for this same type:

class dumb_array
{
    //...
    void swap(dumb_array& other) noexcept
    {
        // Just in case we add UDT members later
        using std::swap;

        // both members are built-in types -> never throw
        swap( this->mArray, other.mArray );
        swap( this->mSize, other.mSize );
    }

    dumb_array& operator=(dumb_array&& other) noexcept
    {
        this->swap( other );
        return *this;
    }
    //...
};

void  swap( dumb_array &l, dumb_array &r ) noexcept  { l.swap( r ); }

A swappable type that needs customization should have a two-argument free function called swap in the same namespace as the type. (The namespace restriction lets unqualified calls to swap to work.) A container type should also add a public swap member function to match the standard containers. If a member swap is not provided, then the free-function swap probably needs to be marked as a friend of the swappable type. If you customize moves to use swap, then you have to provide your own swapping code; the standard code calls the type's move code, which would result in infinite mutual recursion for move-customized types.

Like destructors, swap functions and move operations should be never-throw if at all possible, and probably marked as such (in C++11). Standard library types and routines have optimizations for non-throwable moving types.

This first version of move-assignment fulfills the basic contract. The source's resource markers are transferred to the destination object. The old resources won't be leaked since the source object now manages them. And the source object is left in a usable state where further operations, including assignment and destruction, can be applied to it.

Note that this move-assignment is automatically safe for self-assignment, since the swap call is. It's also strongly exception safe. The problem is unnecessary resource retention. The old resources for the destination are conceptually no longer needed, but here they are still around only so the source object can stay valid. If the scheduled destruction of the source object is a long way off, we're wasting resource space, or worse if the total resource space is limited and other resource petitions will happen before the (new) source object officially dies.

This issue is what caused the controversial current guru advice concerning self-targeting during move-assignment. The way to write move-assignment without lingering resources is something like:

class dumb_array
{
    //...
    dumb_array& operator=(dumb_array&& other) noexcept
    {
        delete [] this->mArray;  // kill old resources
        this->mArray = other.mArray;
        this->mSize = other.mSize;
        other.mArray = nullptr;  // reset source
        other.mSize = 0u;
        return *this;
    }
    //...
};

The source is reset to default conditions, while the old destination resources are destroyed. In the self-assignment case, your current object ends up committing suicide. The main way around it is to surround the action code with an if(this != &other) block, or screw it and let clients eat an assert(this != &other) initial line (if you're feeling nice).

An alternative is to study how to make copy-assignment strongly exception safe, without unified-assignment, and apply it to move-assignment:

class dumb_array
{
    //...
    dumb_array& operator=(dumb_array&& other) noexcept
    {
        dumb_array  temp{ std::move(other) };

        this->swap( temp );
        return *this;
    }
    //...
};

When other and this are distinct, other is emptied by the move to temp and stays that way. Then this loses its old resources to temp while getting the resources originally held by other. Then the old resources of this get killed when temp does.

When self-assignment happens, the emptying of other to temp empties this as well. Then target object gets its resources back when temp and this swap. The death of temp claims an empty object, which should be practically a no-op. The this/other object keeps its resources.

The move-assignment should be never-throw as long as move-construction and swapping are also. The cost of also being safe during self-assignment is a few more instructions over low-level types, which should be swamped out by the deallocation call.

I'm in the camp of those that want self-assignment safe operators, but don't want to write self-assignment checks in the implementations of operator=. And in fact I don't even want to implement operator= at all, I want the default behaviour to work 'right out of the box'. The best special members are those that come for free.

That being said, the MoveAssignable requirements present in the Standard are described as follows (from 17.6.3.1 Template argument requirements [utility.arg.requirements], n3290):

Expression  Return type Return value    Post-condition
t = rv      T&          t               t is equivalent to the value of rv before the assignment

where the placeholders are described as: "t [is a] modifiable lvalue of type T;" and "rv is an rvalue of type T;". Note that those are requirements put on the types used as arguments to the templates of the Standard library, but looking elsewhere in the Standard I notice that every requirement on move assignment is similar to this one.

This means that a = std::move(a) has to be 'safe'. If what you need is an identity test (e.g. this != &other), then go for it, or else you won't even be able to put your objects into std::vector! (Unless you don't use those members/operations that do require MoveAssignable; but nevermind that.) Notice that with the previous example a = std::move(a), then this == &other will indeed hold.

As your current operator= function is written, since you've made the rvalue-reference argument const, there is no way you could "steal" the pointers and change the values of the incoming rvalue reference... you simply can't change it, you could only read from it. I would only see an issue if you were to start calling delete on pointers, etc. in your this object like you would in a normal lvaue-reference operator= method, but that sort of defeats the point of the rvalue-version ... i.e., it would seem redundant to use the rvalue version to basically do the same operations normally left to a const-lvalue operator= method.

Now if you defined your operator= to take a non-const rvalue-reference, then the only way I could see a check being required was if you passed the this object to a function that intentionally returned a rvalue reference rather than a temporary.

For instance, suppose someone tried to write an operator+ function, and utilize a mix of rvalue references and lvalue references in order to "prevent" extra temporaries from being created during some stacked addition operation on the object-type:

struct A; //defines operator=(A&& rhs) where it will "steal" the pointers
          //of rhs and set the original pointers of rhs to NULL

A&& operator+(A& rhs, A&& lhs)
{
    //...code

    return std::move(rhs);
}

A&& operator+(A&& rhs, A&&lhs)
{
    //...code

    return std::move(rhs);
}

int main()
{
    A a;

    a = (a + A()) + A(); //calls operator=(A&&) with reference bound to a

    //...rest of code
}

Now, from what I understand about rvalue references, doing the above is discouraged (i.e,. you should just return a temporary, not rvalue reference), but, if someone were to still do that, then you'd want to check to make sure the incoming rvalue-reference was not referencing the same object as the this pointer.

My answer is still that move assignment doesn't have to be save against self assigment, but it has a different explanation. Consider std::unique_ptr. If I were to implement one, I would do something like this:

unique_ptr& operator=(unique_ptr&& x) {
  delete ptr_;
  ptr_ = x.ptr_;
  x.ptr_ = nullptr;
  return *this;
}

If you look at Scott Meyers explaining this he does something similar. (If you wander why not to do swap - it has one extra write). And this is not safe for self assignment.

Sometimes this is unfortunate. Consider moving out of the vector all even numbers:

src.erase(
  std::partition_copy(src.begin(), src.end(),
                      src.begin(),
                      std::back_inserter(even),
                      [](int num) { return num % 2; }
                      ).first,
  src.end());

This is ok for integers but I don't believe you can make something like this work with move semantics.

To conclude: move assignment to the object itself is not ok and you have to watch out for it.

Small update.

1. I disagree with Howard, which is a bad idea, but still - I think self move assignment of "moved out" objects should work, because swap(x, x) should work. Algorithms love these things! It's always nice when a corner case just works. (And I am yet to see a case where it's not for free. Doesn't mean it doesn't exist though).
2. This is how assigning unique_ptrs is implemented in libc++: unique_ptr& operator=(unique_ptr&& u) noexcept { reset(u.release()); ...} It's safe for self move assignment.
3. Core Guidelines think it should be OK to self move assign.