3

I'm trying to relearn C++, and I tend to want to have an intricate understanding of how everything works (not just "how do I do this"). So I'm wondering why this produces the error it does. Yes, I know that an overloaded assignment operator is supposed to use references (and it works fine if I do), but I'm hoping that an answer to this question might help me learn more about the language rules.

class some_class {
public:
    int n1;
    some_class(int z) : n1(z) { }
    some_class(some_class &x) : n1(x.n1) { }
    some_class operator= (some_class x) { n1 = x.n1; return *this; }
//  some_class & operator= (some_class & x) { n1 = x.n1; return *this; } works fine
};

main () {
    some_class a(10);
    some_class b(20);
    some_class c(30);
    c = b = a;          // error here
}

The compiler (C++03) gives me this, on the c = b = a line:

In function 'int main()':
   error: no matching function for call to 'some_class::some_class(some_class)'
   note: candidates are: some_class::some_class(some_class&)
   note:                 some_class::some_class(int)
   error:   initializing argument 1 of 'some_class some_class::operator=(some_class)'

It's confusing to me, because b = a works fine, and it's looking for a constructor that I'm not legally allowed to declare. I realize that in c = b = a, the b = a part returns a value (not a reference), and that may result in the result being copied to a temporary. But why would c = <temporary> result in a compilation error when b = a wouldn't? Any idea what's going on?

ajb
  • 31,309
  • 3
  • 58
  • 84

5 Answers5

4

Your copy constructor has a non-const reference as its parameter. Temporaries can't be bound to non-const references. When you do:

c = b = a;

This is equivalent (as you say) to:

c.operator=(<temporary>);

It therefore tries to invoke your copy constructor with a temporary whilst initialising the first argument of the call to operator=. This fails for the reason mentioned. A sensible way to fix it is to change the signature of operator= to the more conventional:

some_class& operator=(const some_class& x);

The copy constructor would not then be needed in the implementation of operator=, since the argument to operator= would not be copied. However, copy constructors should generally take a const reference parameter, so you should also change the signature of the copy constructor to:

some_class(const some_class& x);
Stuart Golodetz
  • 20,238
  • 4
  • 51
  • 80
  • 1
    If you are using the copy-and-swap idiom then you want to pass by value to the assignment operator, so I think saying "The correct way" is incorrect. – SirGuy Sep 05 '13 at 17:29
  • 1
    @GuyGreer I would say that you _may_ want the copy assignment operator to take by value, but possibly not always though, because there are compromises (see for example http://stackoverflow.com/questions/18631374/c-copy-construct-parameter-passed-by-value ). Now, I agree that neither is "_The_ correct way" :) – gx_ Sep 05 '13 at 17:39
  • @GuyGreer: Thanks, the wording was too strong - I've changed it. – Stuart Golodetz Sep 06 '13 at 09:53
2

What is causing the error is your copy constructor, which should be

some_class(const some_class&)

This is because you cannot pass a temporary object to a non-const reference, which is what happens in your chained assignment. This is because your assignment operator returns by value which creates a temporary object, which is then passed to the next assignment operator as a value parameter. This invokes the copy constructor, which has a non-const reference parameter and so cannot bind to the temporary object.

Assignment operator should be

some_class& operator= (some_class x)

or

some_class& operator= (const some_class& x)

That is, it takes a value or const reference paramater of the same type and returns a non-const reference, which is *this. Alternatively it can have void return type to prevent chaining.

I know other variations are "allowed", but don't use them unless you know what you are doing.

Unless you have a reason for a value parameter (copy-and-swap idiom for example), you should use const reference to prevent an extra copy.

roalz
  • 2,699
  • 3
  • 25
  • 42
Neil Kirk
  • 21,327
  • 9
  • 53
  • 91
0
some_class(some_class &x) : n1(x.n1) { }
some_class operator= (some_class x) { n1 = x.n1; return *this; }

should be

some_class(const some_class &x) : n1(x.n1) { }
some_class& operator=(const some_class& x) { n1 = x.n1; return *this; }

Using your original signatures, you can't copy or assign from const temporary objects as you show in your sample.

πάντα ῥεῖ
  • 1
  • 13
  • 116
  • 190
0

The key issue is also with unnecessary copy of data if we are not working with references.

There is one more way to solve this, given that you are trying to relearn c++, it would be interesting to know. C++11 have introduced r-value references and std::move to allow working with temporaries in copy constructor and assignment operators to reduce memory copies.

Move copy constructor and assignment operator would look like 

some_class(some_class &&x) : n1(std::move(x.n1)) {}
some_class& operator=(some_class&& x) { n1 = std::move(x.n1); return *this; }
jayadev
  • 3,576
  • 25
  • 13
0

In the statement:

c = b = a;

As assignment operator has Right-to-Left associativity, b = a, is executed first. Since assignment operator returns value, copy constructor is invoked. Since temporary objects cannot be referred to by non-const references (as Stuart Golodetz already mentioned above), the compiler looks for some_class::some_class(some_class x) kind of constructor; and is complaining on not finding it.

The compilation succeeds only if you modify the signature of the copy constructor to:

some_class(const some_class& x);

Assignment operator, as others mentioned, returns a reference traditionally, for the reason that we could chain several of them. It is however not mandatory to design them so.

Sushant Sharma
  • 1
  • 2