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

Ruin

Not sure if the operator= provides the expected semantics: a=b would not modify a. – Estevez

The assignment operator should modify the object behind the this pointer according to the passed object, either member by member or by using some swap function (see copy-and-swap idiom). The fact that it returns something is only required / meaningful if you do stuff like a = b = c or foo(a = b), i.e. use the result of the assignment expression, but that's not the "main task", so to speak. It's modifying the data of the this pointer, and you're not doing that. – Prism

For me, it's just bullshit... or unintentional obfuscation. – Lepus

A swap() function is not yet implemented. And I had already changed the assignment operator to return a reference. When it still didn't work, I used the compiler-generated version. Then Your answers showed me why the custom version was wrong – Ruin

-edit time was over- which will help me to provide a new custom one if needed. – Ruin

Answer 1

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

Underline

Answer 2

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

Olympium

Taking the parameter by value violates the usual coding conventions, which says to pass class types by const reference. Which means that you should probably avoid it. – Contentment

@JamesKanze Want speed? Pass by value. It's of course far from a universal advice, but it applies perfectly here. And it saves you from having to provide both a copy assignment op and a move assignment op. – Olympium

You might also point out that the member swap must not raise any exceptions (otherwise, you've defeated the purpose). (And the using std::swap seems a bit superfluous; I find it much clearer to write std::swap for each invocation.) – Contentment

@JamesKanze: That coding convention is old fashioned - passing by value if you actually need a copy improves exception safety and gives the compiler better opportunities for optimizations – Ecklund

@JamesKanze You're right about the nothrow part, I will add that. But for using std::swap;, this is a standard idiom to allow ADL to kick in for classes defining their own global swap. Of course, it's pointless for int, but as it's a standard idiom, I tend to use it everywhere. – Olympium

@DieterLücking - compromises with everything - passing by value when you need a copy means interface changes if you later find a way to implement it without copying, and that's a big problem sometimes (e.g. a breaking change for shared library APIs). The cost of copying internally is small and less variable. So, a rule of thumb good for intra-library / -app use may be poor for the interfaces between. – Toneytong

@DieterLücking It also exposes an implementation detail in the public interface. Of course, one might argue that the ubiquitous rule is bad to begin with; using pass by const reference is a premature optimization. The important point, however, is that whatever the convention, it should be independent of what you put in the function. (For example, in his exact case, you normally wouldn't use the swap idiom for assignment, because it is just excess complexity. But you don't want to have to change the public signature if this changes later.) – Contentment

@Angew If I understand it correctly, ADL should kick in anyway. And you don't want to blindly use std::swap, if a member hasn't defined its own swap, since that won't (necessarily) be non throwing. The swap idiom doesn't work automatically; before using it, you must know how to swap each of the individual elements in a non-throwing manner, and implement the swap function to do so. – Contentment

@JamesKanze If you specify std::swap, ADL will not kick in - it applies to unqualified calls only. And note that even std::swap is not necessarily non-throwing. – Olympium

@Angew But you only specify std:: when you explicitly want the version from the standard library, for example, for pointers. For class types, you'll specify what the documentation says to use: most of the times, member.swap(). In all cases, you must be very sure about what you're getting, or you'll risk loosing the no throw guarantee. – Contentment

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.

Contentment

-1 "a correctly written assignment operator will always return a reference" is incorrect. It is incorrect even when it's interpreted as referring to the copy assignment operator only. There is one very limited context in which the statement is correct, namely when declaring a copy or move assignment operator as deleted or defaulted. – Pilch

+1 from me... I don't want anything unexpected unless there's a bloody good reason - something warranting special casing. – Toneytong

@Alf Your statement is wrong. A correctly implemented assignment operator will return a reference. The standard doesn't require it, but there are a lot of things the standard doesn't require which are necessary for the code to be correct. – Contentment

@JamesKanze The standard even does require it for types used as CopyAssignable or MoveAssignable by the standard library (C++11 [moveassignable] + [copyassignable]). – Olympium

@JamesKanze: re-assering the incorrect proposition doesn't make it less wrong. also, it's pretty silly to do so. essentially that says, "i have no arguments or evidence and i don't care about logic either". – Pilch

@Angew: thanks, you have identified a breaking change in C++11 (it breaks valid C++03 code). I suggest you submit a formal defect report. At least, with a quick browse I was unable to find any existing DR on this. – Pilch

@Cheersandhth.-Alf [C++03 [lib.container.requirements]§4 Table 64] lists a return type of T& for the Assignable requirement as well. – Olympium

@Angew: oh, so it does! you're the first to note that. since this has been discussed extensively without anyone noticing, i was sure there was no such. thanks. – Pilch

@Cheersandhth.-Alf You can just do a trivial edit yourself (e.g. add a trailing space somewhere) ;-) – Olympium

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?

Macaque

Except for the purpose of declaring the copy assignment operator as default or delete, it is IMHO far better to let it have void return type. First, one avoids support bad programming practice, namely expressions with side effects. Secondly, the function declaration becomes shorter and more clear. Third, the implementation code becomes shorter and more clear. So, the "should return a reference" is at best an arguable opinion. But since I've never seen it argued other than purely associative (that's what I've always seen, etc.), I think objectively void is better. ;-) – Pilch

@Cheersandhth.-Alf: out of interest, do you consistently suppress the default copy assignment for that reason? Seems like quite an uphill battle to wage against a language that "wants" sub-expressions with side effects. Use Haskell ;-) – Underline

@Cheersandhth.-Alf Using void makes it behave differently than the built-in versions do, and that's always a bad thing when overloading operators to mimick the built-in purpose. And it disabled expressions like a = b = c;, which are quite useful in certain contexts. – Olympium

@Angew: at risk of recapitulating an old argument, there is a programming style which states that a = b = c; is never preferable to b = c; a = b;, and that (a = b) = c; is beyond disgusting. Obviously you don't have to use that style, but if you do use it it's natural to conclude that mutators certainly should not return lvalues regardless of what the designers of C++ think, and generally shouldn't return anything. – Underline

@SteveJessop: no, it's just a target of oppportunity. ;-) – Pilch

+1 (earlier) for a direct, simple and efficient solution with a sensible link to a bells-and-whistles version for cases where it's actually useful, and hear hear for "always a bad thing" in your comment. – Toneytong

@Cheersandhth.-Alf Actually, the standard requirements CopyAssignable and MoveAssignable require the return type to be a non-const reference (C++11 [utility.arg.requirements] Table 22 + 23). So if you want to use your class with the standard library without hitting potential UB, you have to return a reference. – Olympium