5

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
Improve this question
5
  • 1
    Not sure if the operator= provides the expected semantics: a=b would not modify a. Commented Mar 27, 2014 at 10:43
  • 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. Commented Mar 27, 2014 at 10:47
  • For me, it's just bullshit... or unintentional obfuscation. Commented Mar 27, 2014 at 10:49
  • 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 Commented Mar 27, 2014 at 12:35
  • -edit time was over- which will help me to provide a new custom one if needed. Commented Mar 27, 2014 at 12:48

4 Answers 4

Reset to default
15

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

Improve this answer
Sign up to request clarification or add additional context in comments.

10 Comments

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

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

Improve this answer

Comments

2

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?

Improve this answer

7 Comments

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. ;-)
@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 ;-)
@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.
@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.
@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.
|
2

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.

Improve this answer

9 Comments

-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.
+1 from me... I don't want anything unexpected unless there's a bloody good reason - something warranting special casing.
@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.
@JamesKanze The standard even does require it for types used as CopyAssignable or MoveAssignable by the standard library (C++11 [moveassignable] + [copyassignable]).
@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".
|

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.