Why can't I use a copy constructor inside of my assignment operator?

Question

Title kind of hits the mark.

 // copy constructor IntList::IntList(const IntList& source){ Node* currentNode = source.first; first = nullptr; while(currentNode){ append(currentNode->info); currentNode = currentNode->next; } } //Assignment operator should copy the list from the source //to this list, deleting/replacing any existing nodes IntList& IntList::operator=(const IntList& source){ this -> ~IntList(); this = new IntList(source); return *this; }

Error: intlist.cpp:26:30: error: lvalue required as left operand of assignment this = new IntList(source);

The destructor properly deletes each node in the linked list. Technically "this" would be whatever linked list was passed in with the assignment operator. Isn't that treated as an object that can be set?

this -> ~IntList(); -- This is wrong. This should not be done in your class whatsoever. A simple copy/swap of the members is all you need to do, without any calls to new. — PaulMcKenzie
– PaulMcKenzie, Commented Apr 30, 2021 at 3:00
At a purely mechanical level, this is not modifiable. this = whatever; is illegal. — Pete Becker
– Pete Becker, Commented Apr 30, 2021 at 3:11

alfC · Accepted Answer · 2021-04-30 11:11:00Z

The short answer is that you can't assign this. (this is the pointer to the current object it cannot be "relocated", it is a "const" pointer for all purposes).

What you mean is something like:

 new(this) IntList(source);

Which is called "placement" new:

But you don't need to learn about this because all this is bad anyway. Keep reading...

Yours is an old and bad design pattern to implement assignment as destruction + construction.

These days, unless there is a better shortcut (that depends on you specific design) we implement swap (also useful) and then operator= from copy(construct)+swap.

(Besides any explict call to the destructor smells bad.)

IntList& IntList::operator=(IntList const& source){ IntList tmp(source) this->swap(tmp); return *this; }

The main point is not only more elegant code, ultimately a side effect is that you are writing exception safe code by default. (think of what happens if the new object cannot be constructed in your original code: the *this object is left in an invalid "destructed" state.)

See the series of talks: https://www.youtube.com/watch?v=W7fIy_54y-w

This is really advanced now and it gets weird (as discussed with @PaulMcKenzie). It turns out that (I think for some quirks only in C++14), that you can omit the temporary and transfer that into the argument itself passed by value:

IntList& IntList::operator=(IntList source) noexcept{ return swap(source); }

Although it can be obscure at first.

It is less code
it is noexcept: it can't fail!. The operation call can fail but not inside the function
this provides move assignment if there a move constructor, so you don't need to write a separate move assignment (even less code).

(Incidentally, I think this shows why member swap should return *this)

This opens an intriguing question: if I have copy constructor and swap, shouldn't the compiler implement operator= automatically for me? (not the old-C operator= member-by-member, but one that knows about the semantics of the class implemented like shown). IntList& operator=(IntList) = default;

Finally, don't miss the forest for the tree.

You wanted to implement assignment without writing new code, assuming that your copy constructor was good and that is all you got.

However this is still not always optimal!

For containers copy construction usually involves a) (unconditional) allocation and b) a sequence of "uninitialized copies" (ultimately a form of placement new). Both a) and b) are slower than A) no-allocation and B) a sequence of assignment respectively.

This means that, if for some reason you know that you don't need to allocate (e.g. because the source "fits" in your allocated memory already), and that you can assign element by element (faster than new-place element by element) you might be in good shape to get a more optimal solution by writing a custom operator= that takes into account all this.

Thank you for this... Wouldn't it still be necessary to call the destructor on the old IntList just in case it was stored in the heap... ie: IntList listA = new IntList(parameters); IntList listB(parameters); listA = listB; If you don't call the destructor on 'this', aren't you going to have a memory leak if you're passing in anything from the heap?
@JustinKasowski IntList listA = new IntList(parameters); IntList listB(parameters); listA = listB;. This is totally wrong. You are assigning an object to a pointer. You should learn about how pointers and dynamic memory allocations work, at best by reading some good book about C++ for beginners.
@JustinKasowski -- If you look at the code, where would there be a memory leak? No calls to new were done, everything is value-based. The destructor of tmp is called when tmp goes out of scope automatically, delete[]-ing the old memory. That's the purpose of the destructor.
The original IntList that's being overloaded could potentially have been made with 'new'. edit I get it now, you swap whatever IntList you're passing in with tmp and then tmp is cleared. Don't know why I blanked on that
@JustinKasowski -- Then that's the coder's "fault" to have used new to create it. It has absolutely nothing to do with the internals of IntList itself, which is what your question and the answer given deals with.

PaulMcKenzie · Accepted Answer · 2021-04-30 03:15:00Z

If your copy constructor and destructor for IntList are working properly, the easiest way to implement the assignment operator is to use the copy/swap idiom:

#include <algorithm> //... IntList& IntList::operator=(const IntList& source) { if (&source != this) { IntList temp(source); std::swap(temp.first, first); // add swaps for each member //... } // <-- This is where the old contents will be destroyed, // when `temp` goes out of scope. No need for this->~IntList(); return *this; }

In your code, doing this:

this->~IntList();

is not necessary, and probably is wrong anyway. Code like that is reserved for usage with placement-new, and your code doesn't show any usage of placement-new.

The conditional check is semantically unnecessary, and a dubious optimization. Unless you like to write my_list = my_list; very often or your algorithms are designed to do unnecessary indirect self assignments.
I know that the self check is not necessary. But I have no idea if creating IntList is expensive or not, since the OP didn't post its details. There is no harm in making this check.
Yes, the harm is to do a check every time, just because every blue moon you might do an ill conceived self-assignment. I stand by this regardless of whether IntList is expensive to construct/copy or not.
Then if this is your concern, then assignment operator should have been: IntList& IntList::operator=(IntList source) with no self-check.

Muhammad Hassan · Accepted Answer · 2021-04-30 02:54:31Z

A common mistake is to call the assignment operator from the copy constructor. Take the Array class for example. Inside the copy you might think it's simple to write

Collectives™ on Stack Overflow

Why can't I use a copy constructor inside of my assignment operator?

3 Answers 3

9 Comments

5 Comments

1 Comment

Linked

Hot Network Questions