views:

1135

answers:

6

Since a copy constructor

MyClass(const MyClass&);

and an = operator overload

MyClass& operator = (const MyClass&);

have pretty much the same code, the same parameter, and only differ on the return, is it possible to have a common function for them both to use?

A: 

It's common to have copy constructor use this.operator=() to copy fields.

Victor Sergienko
It might be common (heaven forbid), but it's a bad idea.
sbi
@Victor: I don't think that's true. At least it shouldn't be.
sellibitze
+14  A: 

Yes. There are two common options. One - which I don't recommend - is to call the operator= from the copy constructor explicitly:

MyClass(const MyClass& other)
{
    operator=(other);
}

However, providing a good operator= is a challenge when it comes to dealing with the old state and issues arising from self assignment. Also, all members and bases get default initialized first even if they are to be assigned to from other. This may not even be valid for all members and bases and even where it is valid it is semantically redundant and may be practically expensive.

An increasingly popular solution is to implement operator= using the copy constructor and a swap method.

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

or even:

MyClass& operator=(MyClass other)
{
    swap(other);
    return *this;
}

A swap function is typically simple to write as it just swaps the ownership of the internals and doesn't have to clean up existing state or allocate new resources.

Advantages of the copy and swap idiom is that it is automatically self-assignment safe and - providing that the swap operation is no-throw - is also strongly exception safe.

To be strongly exception safe, a 'hand' written assignment operator typically has to allocate a copy of the new resources before de-allocating the assignee's old resources so that if an exception occurs allocating the new resources, the old state can still be returned to. All this comes for free with copy-and-swap but is typically more complex, and hence error prone, to do from scratch.

The one thing to be careful of is to make sure that the swap method is a true swap, and not the default std::swap which uses the copy constructor and assignment operator itself.

Typically a memberwise swap is used. std::swap works and is 'no-throw' guaranteed with all basic types, pointer types and std::auto_ptr. Most smart pointers can also be swapped with a no-throw guarantee.

Charles Bailey
Actually, they are not common operations. While the copy ctor first-time initializes the object's members, the assignment operator overrides existing values. Considering this, alling `operator=` from the copy ctor is in fact pretty bad, because it first initializes all values to some default just to override them with the other object's values right afterwards.
sbi
I said common options, not operations. I completely agree that calling `operator=` from a copy constructor is not good but you only have to look at a reasonable about of real world code to see how common it is.
Charles Bailey
Downvoters, care to explain?
Charles Bailey
Um, didn't you just respond to my explanation?
sbi
Maybe to "I don't recommend", add "and neither does any C++ expert". Someone might come along and fail to realise that you aren't just expressing a personal minority preference, but the settled consensus opinion of those who've actually thought about it. And, OK, maybe I'm wrong and some C++ expert does recommend it, but personally I'd still lay down the gauntlet for someone to come up with a reference for that recommendation.
Steve Jessop
@sbi: Yes, I replied to your comment. IMHO both options are very common, but you disagree. Popularity is to some extent a matter of opinion so I'm happy to disagree on that point but I'm interested to know why I've been downvoted which should mean 'wrong or unhelpful' which wasn't my intention.
Charles Bailey
@Steve 'onebyone' Jessop: I don't like to sound too prescriptive, people use C++ happily (or not so happily) in many different ways. I added the 'I don't recommend' after re-reading my answer because I felt that my disapproval probably wasn't shown strongly enough by just the contents of the next paragraph.
Charles Bailey
Fair enough, I upvoted you already anyway :-). I figure that if something is widely considered best practice, then it's best to say so (and look at it again if someone says it's not really best after all). Likewise if someone asked "is it possible to use mutexes in C++", I wouldn't say "one fairly common option is to completely ignore RAII, and write non-exception-safe code that deadlocks in production, but it's increasingly popular to write decent, working code" ;-)
Steve Jessop
+1. And i think there is always analysis in need. I think it's reasonable to have an `assign` member function used by both the copy ctor and assignment operator in some cases (for lightweight classes). In other cases (resource intensive/using cases, handle/body) a copy/swap is teh way to go of course.
Johannes Schaub - litb
@litb: Agreed, e.g. if you've just got a couple of basic types in a class then assigning your members won't even have any self-assignment or exception-safe issues in any case so there's little point in sweating about the most robust solution. As soon as your constructors `new` or do any sort of resource allocation you start to have to consider your options a bit more carefully.
Charles Bailey
@litb: If the class is light-weight, does copy-swap do any harm?
sbi
Yes, since then you don't need a special swap, and i don't see what it buys you wrt exception safety. You would just add one useless function (swap member function), and create one useless copy (since swap/copy would be essentially the same speed here, this is then useless in the real sense). But i might be wrong of course, i'm just don't seeing the point in doing copy-swap then.
Johannes Schaub - litb
Of course, it depends on what exactly "light-weight" is - i meant a class that doesn't do dynamic memory management or holding of resources. The need for user defined cctor or assignment ops in this case is rare i think - but it might occasionally come up.
Johannes Schaub - litb
Not going to downvote you, but i've never seen a copy constructor implemented in terms of `operator=` and it sure looks inefficient.
rlbond
@rlbond: I can assure you that it is depressingly common. I've never seen it in code that would otherwise have made me think: 'This is some really clean c++, here!', though.
Charles Bailey
In fact, lemme quote Herb Sutter: "If anything, the idiom is exactly backwards: copy construction should be implemented in terms of copy assignment, rather than the reverse." -> http://www.gotw.ca/gotw/023.htm . Of course that article is old, but it shows it's not all that crazy of a thought :)
Johannes Schaub - litb
Forgive me if my lack of knowledge is showing, but how, in the swap solution, is the initial destruction of the target ensured?
MPelletier
@litb: I was surprised by this so I looked up Item 41 in Exception C++ (which this gotw turned into) and this particular recommendation has gone and he recommends copy-and-swap in its place. Rather sneakily he has dropped "Problem #4: It's Inefficient for Assignment" at the same time.
Charles Bailey
@MPelletier: The swap action moves the old state of the target into the temporary copy (as the copy's state is moved into the target). As the initial copy is local to the assignment operator it goes out of scope at the end of the function body, so ensuring that the old state of the target is detroyed.
Charles Bailey
@Charles, ah that's good. I expected no different thing from him, now i know he actually did "fix" that :) Nontheless, i don't think it's particularly "bad" to call a common function from both the cctor and copy assignment operator, like some folks here say. Why do it the complex way if an easy solution suffices.
Johannes Schaub - litb
+1 Sutter also covers copy-swap in a later gotw: http://www.gotw.ca/gotw/059.htm
sdtom
A: 

Yes. You may have some private function Copy that would be used by both of them. Or you can implement copy construcor in terms of assignment operator. There is alternative somewhat controversial technique in my opinion. You may check it out here.

BostonLogan
see my comments to Charles why this is a bad idea.
sbi
@sbi, While calling `operator=` from the copy ctor is bad, would you agree factoring out the code that *only* worries about setting values into an `assign` function that's called from the cctor and assignment op wouldn't be too evil? That way, `operator=` will do the cleaning, then call `assign`, and the cctor will call `assign` directly after setting things up.
Johannes Schaub - litb
NO, I'm not agreeing. What does it buy you over the copy-swap idiom, except for many developers to get this wrong?
sbi
+5  A: 

The copy constructor performs first-time initialization of objects that used to be raw memory. The assignment operator, OTOH, overrides existing values with new ones. More often than never, this involves dismissing old resources (for example, memory) and allocating new ones.

If there's a similarity between the two, it's that the assignment operator performs destruction and copy-construction. Some developers used to actually implement assignment by in-place destruction followed by placement copy-construction. However, this is a very bad idea. (What if this is the assignment operator of a base class that called during assignment of a derived class?)

What's usually considered the canonical idiom nowadays is using swap as Charles suggested:

MyClass& operator=(MyClass other)
{
    swap(other);
    return *this;
}

This uses copy-construction (note that other is copied) and destruction (it's destructed at the end of the function) -- and it uses them in the right order, too: construction (might fail) before destruction (must not fail).

sbi
A: 

Something bothers me about:

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

First, reading the word "swap" when my mind is thinking "copy" irritates my common sense. Also, I question the goal of this fancy trick. Yes, any exceptions in constructing the new (copied) resources should happen before the swap, which seems like a safe way to make sure all the new data is filled before making it go live.

That's fine. So, what about exceptions that happen after the swap? (when the old resources are destructed when the temporary object goes out of scope) From the perspective of the user of the assignment, the operation has failed, except it didn't. It has a huge side effect: the copy did actually happen. It was only some resource cleanup that failed. The state of the destination object has been altered even though the operation seems from the outside to have failed.

So, I propose instead of "swap" to do a more natural "transfer":

MyClass& operator=(const MyClass& other)
{
    MyClass tmp(other);
    transfer(tmp);
    return *this;
}

There's still the construction of the temporary object, but the next immediate action is to free all current resources of the destination before moving (and NULLing so they won't be double-freed) the resources of the source to it.

Instead of { construct, move, destruct }, I propose { construct, destruct, move }. The move, which is the most dangerous action, is the one taken last after everything else has been settled.

Yes, destruction fail is a problem in either scheme. The data is either corrupted (copied when you didn't think it was) or lost (freed when you didn't think it was). Lost is better than corrupted. No data is better than bad data.

Transfer instead of swap. That's my suggestion anyway.

Matthew