calling copy constructor in assignment operator

Question

In an already existing class of a project I am working on I encountered some strange piece of code: The assignment operator calls the copy constructor.

I added some code and now the assignment operator seems to cause trouble. It is working fine though if I just use the assignment operator generated by the compiler instead. So I found a solution, but I'm still curious to find out the reason why this isn't working.

Since the original code is thousands of lines I created a simpler example for you to look at.

#include <iostream>
#include <vector>

class Example {

private:
  int pValue;
public:
  Example(int iValue=0)
  {
    pValue = iValue;
  }

  Example(const Example &eSource)
  {
    pValue = eSource.pValue;
  }

  Example operator= (const Example &eSource)
  {
    Example tmp(eSource);
    return tmp;
  }

  int getValue()
  {
    return pValue;
  }

};

int main ()
{
  std::vector<Example> myvector;

  for (int i=1; i<=8; i++) myvector.push_back(Example(i));

  std::cout << "myvector contains:";
  for (unsigned i=0; i<myvector.size(); ++i)
    std::cout << ' ' << myvector[i].getValue();
  std::cout << '\n';

  myvector.erase (myvector.begin(),myvector.begin()+3);

  std::cout << "myvector contains:";
  for (unsigned i=0; i<myvector.size(); ++i)
    std::cout << ' ' << myvector[i].getValue();
  std::cout << '\n';

  return 0;
}

The output is

myvector contains: 1 2 3 4 5

but it should be (an in fact is, if I just use the compiler-generated assignment operator)

myvector contains: 4 5 6 7 8

Answer 1

Your operator= does not do what everyone (including the standard library) thinks it should be doing. It doesn't modify *this at all - it just creates a new copy and returns it.

It's normal to re-use the copy constructor in the copy assignment operator using the copy-and-swap idiom:

Example& operator= (Example eSource)
{
  swap(eSource);
  return *this;
}

Notice how the operator takes its parameter by value. This means the copy-constructor will be called to construct the parameter, and you can then just swap with that copy, effectively assigning to *this.

Also note that it's expected from operator= to return by reference; when overloading operators, always follow the expected conventions. Even more, the standard actually requires the assignment operator of a CopyAssignable or MoveAssignable type to return a non-const reference (C++11 [moveassignable] and [copyassignable]); so to correctly use the class with the standard library, it has to comply.

Of course, it requires you to implement a swap() function in your class:

void swap(Example &other)
{
  using std::swap;
  swap(pValue, other.pValue);
}

The function should not raise exceptions (thanks @JamesKanze for mentioning this), no to compromise the exception safety of the operator=.

Also note that you should use the compiler-generated default constructors and assignment operators whenever you can; they can never get out of sync with the class's contents. In your case, there's no reason to provide the custom ones (but I assume the class is a simplified version for posting here).

Answer 2

The assignment operator you found is not correct. All it does is make a copy of eSource, but it's supposed to modify the object on which it is called.

The compiler-generated assignment operator for the class is equivalent to:

Example &operator= (const Example &eSource)
{
    pValue = eSource.pValue;
    return *this;
}

For this class there's no point implementing operator=, since the compiler-generated version basically cannot be improved on. But if you do implement it, that's the behaviour you want even if you write it differently.

[Alf will say return void, most C++ programmers will say return a reference. Regardless of what you return, the vital behaviour is an assignment to pValue of the value from eSource.pValue. Because that's what the copy assignment operator does: copy from the source to the destination.]

Answer 3

Probably the most frequent correct implementation of operator= will use the copy constructor; you don’t want to write the same code twice. It will do something like:

Example& Example::operator=( Example const& other )
{
    Example tmp( other );
    swap( tmp );
    return *this;
}

The key here is having a swap member function which swaps the internal representation, while guaranteeing not to throw.

Just creating a temporary using the copy constructor is not enough. And a correctly written assignment operator will always return a reference, and will return *this.

Answer 4

First of all, operator=() should return a reference:

Example& operator=(const Example& eSource)
{
    pValue = eSource.pValue;
    return *this;
}

Mind that your version returns a copy of tmp so in fact it performs two copies.

Second of all, in your class there's no need to even define custom assignment operator or copy constructor. The ones generated by compiler should be fine.

And third of all, you might be interested in copy-and-swap idiom: What is the copy-and-swap idiom?