Move Assignment Operator and 'If (This != &Rhs)'

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

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.

Trying to Write Move Constructor in terms of Move Assignment

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 ...)

Overload assignment operator and rule of zero

According to my understanding, adding a operator= overload will not prevent the compiler from generating the default one according to the rule of 0.

I base this understanding on the fact that your operator= overload is not in fact a copy assignment, nor a move assignment. Therefore the rules about generaing default constructors and assignment operators are not relevant.

I verified it with MSVC.

You can use the code below to verify with your compiler:

#include <iostream>

template <typename T>
struct B
{
    B(T const & n) : bn(n) {}
    T bn{ 0 };
};

template <typename T>
struct A
{
    A(T const & n) : an(n) {}
    A<T>& operator=(const B<T>& rhs)
    {
        an = rhs.bn;
        return *this;
    }
    T an{ 0 };
};

int main()
{
    A<int> a1{ 5 };
    A<int> a2{ 6 };
    std::cout << a2.an << ",";
    a2 = a1;    // Use default assinment
    std::cout << a2.an << ",";
    B<int> b{ 3 };
    a2 = b;     // Use custom assignment
    std::cout << a2.an << std::endl;
    return 0;
}

The output should be: 6,5,3:

6 is the value A<int> a2 is constructed with, 5 is the value assigned from A<int> a1, and 3 is the value assigned from B<int> b.

Note: an alternative would be to use a user-defined conversion function, as @LouisGo commented (see above).

Strange Move Assignment Operator Signature

Objects of a class used as expressions can be rvalues or lvalues. The move assignment operator is a member function of a class.

This declaration

Tensor & Tensor::operator=(Tensor && rhs) &&

means that this move assignment operator is called for rvalue object of the class.

Here is a demonstrative program.

#include <iostream>

struct A
{
    A & operator =( A && ) &
    {
        std::cout << "Calling the move assignment operator for an lvalue object\n";
        return *this;
    }

    A & operator =( A && ) &&
    {
        std::cout << "Calling the move assignment operator for an rvalue object\n";
        return *this;
    }
};

int main() 
{
    A a;

    a = A();

    A() = A();

    return 0;
} 

The program output is

Calling the move assignment operator for an lvalue object
Calling the move assignment operator for an rvalue object

That is in this statement

    a = A();

the left hand operand of the assignment is an lvalue.

In this statement

    A() = A();

the left hand operand of the assignment is rvalue (a temporary object).

Why move assignment operator should return reference to *this

Returning Foo is potentially expensive, and impossible if the type isn't copyable. It's also surprising: (a=b)=c would create a temporary and assign c to that, when you'd expect both assignments to be to a.

Returning Foo&& is just weird; you don't want things mysteriously turning into rvalues so that e.g. f(a=b) unexpectedly moves from a without you telling it to.

Returning Foo& is the conventional way to make the operator behave in an unsurprising way if you chain it.

Move assignment operator for class with dynamic bindings

wxEvtHandler is not movable, so you can't make a class derived from it movable. The simplest solution is, of course, just not inherit from it.

What is wrong with this operator overloading code?

While the actual crash may be caused by the mismatched deallocation of the array as mentioned by rlbond's answer, there is another subtler alternative:

operator= returns a copy (not a reference as is conventional), which created using the default copy constructor. In this example it is not used and so a temporary is created. The temporaries x and y are pointing to the same memory as the copy, since the copy constructor is not defined. When the temporary is destroyed its x and y are deleted - then later o2 is destroyed resulting in the same memory getting deallocated twice.

It's worth noting that there's still a lot that can go wrong with this class, and it needs some further love and attention. Also worth seeing how we can find this error.

Here's what I get when pushing it through the pc-lint online linter.
Note that this includes the problem causing the crash, and a lot of other things that need fixing.

 1  #include <iostream>
 2  
 3  using namespace std;
 4  
 5  class MyClass
 6  {
 7  private:
 8      int *x;
 9      int *y;
10  public: 
11      MyClass()
12      {
13          x = new int[1]; *x = 0;
14          y = new int[1]; *y = 0;
15      }
16      ~MyClass()
17      {
18          if(x != NULL)
19          {
20              delete x;
21              x = NULL;
22          }
23          if(y != NULL)
24          { 
25              delete y;
26              y = NULL;
27          }
28      }
29      MyClass operator=(MyClass & rhs)
        _
30      {

diy.cpp 30 Info 1722: assignment operator for class 'MyClass' does not return a reference to class

diy.cpp 30 Info 1720: assignment operator for class 'MyClass' has non-const parameter

31          MyClass temp;
32          temp.Set(rhs.x[0], rhs.y[0]);
33          return temp;
34      }
35      void Set(int a, int b)
36      {
37          if(x != NULL)
38          {           
39              *x = a;
40          }
41          if(y != NULL)
42          {           
43              *y = b;
44          }
45      }
46      void Show()
47      {
48          cout<<x[0]<<y[0]<<endl;
49      }
                _
13          x = new int[1]; *x = 0;

diy.cpp 13 Info 1733: new in constructor for class 'MyClass' which has no copy constructor

20              delete x;

diy.cpp 20 Warning 424: Inappropriate deallocation (delete) for 'new[]' data

25              delete y;

diy.cpp 25 Warning 424: Inappropriate deallocation (delete) for 'new[]' data

33          return temp;

diy.cpp 33 Info 1772: Assignment operator 'MyClass::operator=(MyClass &)' is not returning *this

34      }

diy.cpp 34 Warning 1529: Symbol 'MyClass::operator=(MyClass &)' not first checking for assignment to this

diy.cpp 34 Info 1764: Reference parameter 'rhs' (line 29) could be declared const ref

diy.cpp 34 Warning 1539: member 'MyClass::x' (line 8) not assigned by assignment operator

diy.cpp 34 Warning 1539: member 'MyClass::y' (line 9) not assigned by assignment operator

48          cout<<x[0]<<y[0]<<endl;

diy.cpp 48 Warning 613: Possible use of null pointer 'MyClass::x' in left argument to operator '[' [Reference: file diy.cpp: line 37]

diy.cpp 48 Warning 613: Possible use of null pointer 'MyClass::y' in left argument to operator '[' [Reference: file diy.cpp: line 41]

49      }

diy.cpp 49 Info 1762: Member function 'MyClass::Show(void)' could be made const

50  };
51  
52  int main()
53  {
54      MyClass o1;
55      o1.Set(10, 20);
56      o1.Show();
57  
58      MyClass o2;
59      o2 = o1;
60      o2.Show();
61  }
62  

---

Related Topics