33

Are there some drawbacks of such implementation of copy-constructor?

Foo::Foo(const Foo& i_foo)
{
   *this = i_foo;
}

As I remember, it was recommend in some book to call copy constructor from assignment operator and use well-known swap trick, but I don't remember, why...

stas
  • 631
  • 2
  • 7
  • 9
  • 1
    possible duplicate of http://stackoverflow.com/questions/1533725/is-it-bad-form-to-call-the-default-assignment-operator-from-the-copy-constructor – sth Apr 14 '10 at 16:12
  • Another duplicate: http://stackoverflow.com/questions/1457842/is-this-good-code-copy-ctor-operator Related: http://stackoverflow.com/questions/1477145/reducing-code-duplication-between-operator-and-the-copy-constructor http://stackoverflow.com/questions/1734628/copy-constructor-and-operator-overload-in-c-is-a-common-function-possible http://stackoverflow.com/questions/2034635/explicit-copy-constructor-or-implicit-parameter-by-value – sth Apr 14 '10 at 16:22
  • Which book would that be? It is good practice to call common code, probably in a named private function, to perform copying. But to use the assignment operator form the copy constructor? In most cases - no. –  Apr 14 '10 at 16:35

3 Answers3

23

Yes, that's a bad idea. All member variables of user-defined types will be initialized first, and then immediately overwritten.

That swap trick is this: 

Foo& operator=(Foo rhs) // note the copying
{
   rhs.swap(*this); //swap our internals with the copy of rhs
   return *this;
} // rhs, now containing our old internals, will be deleted
sbi
  • 219,715
  • 46
  • 258
  • 445
  • 10
    "that's a bad idea," is too broad a statement IMO. By saying this, you are potentially micro-optimizing prematurely. If you write assignment code in both the ctor and `op=()`, you create a duplication of code and violate DRY. Figure out if the costs involved in double-initialization, compare them to the benefits gained by reducing the complexity in the code base, and then decide which way is right. – John Dibling Apr 14 '10 at 16:24
  • 2
    The nice thing about the swap trick is that it deals automatically with self-assignment, and (assuming `.swap()` doesn't throw) is strongly exception-safe, in that either the assignment succeeds or it's left unchanged and an exception is thrown. – David Thornley Apr 14 '10 at 16:42
  • 4
    Except these days, the trick is done better by passing rhs by value. See http://cpp-next.com/archive/2009/08/want-speed-pass-by-value/ – Kaz Dragon Apr 14 '10 at 16:47
  • 1
    @Kaz: You're right. `` That's twice these days that I had to admit that I'm doing something mainly out of old habit, not why it is (still) considered state of the art. Maybe I'm getting to old. – sbi Apr 14 '10 at 22:44
  • 2
    @JohnDibling calling op=() from copy ctor is not the only way to avoid code duplication, and I think among all the techniques, it is still not a very good one. – weidi Jun 21 '12 at 10:35
  • how about Foo is a abstract class? – camino Feb 27 '15 at 22:46
  • @camino: What then? Assignment in an abstract base class? Sounds fishy. – sbi Feb 27 '15 at 22:50
  • @sbi if Foo is an abstract class with data members, so we could not use the swap trick. The example could find here: http://www.gamedev.net/topic/526089-c-data-members-in-an-abstract-class/ – camino Feb 28 '15 at 01:34
  • @camino: What you ask makes no sense. Assignment is for value classes, not for polymorphic base classes. Are you perhaps confusing values and references? – sbi Feb 28 '15 at 02:05
  • @sbi, When you copy an object, you need to copy all parts of the object, right? suppose Bar is derived from Foo, and Foo is abstract class (has pure virtual function), so the function "Foo& operator=(Foo rhs)" will report a compiler error – camino Feb 28 '15 at 02:31
  • @camino: If `Foo` it's an abstract base class, then you will refer to derived classes though `Foo` pointers and references, because that's what polymorphic class hierarchies are for. And you cannot assign values through references. (It just doesn't make sense to assign an automobile to an airplane through two vehicle references.) Assignment is for value classes. if inheritance is involved with this, then (hopefully) there's no virtual functions involved, and thus no abstract base classes. (In polymorphic class hierarchies, you need to `clone()`, rather than copy.) – sbi Feb 28 '15 at 10:31
11

There are both potential drawbacks and potential gains from calling operator=() in your constructor.

Drawbacks:

  • Your constructor will initialize all the member variables whether you specify values or not, and then operator= will initialize them again. This increases execution complexity. You will need to make smart decisions about when this will create unacceptable behavior in your code.

  • Your constructor and operator= become tightly coupled. Everything you need to do when instantiating your object will also be done when copying your object. Again, you have to be smart about determining if this is a problem.

Gains:

  • The codebase becomes less complex and easier to maintain. Once again, be smart about evaluating this gain. If you have a struct with 2 string members, it's probably not worth it. On the other hand if you have a class with 50 data members (you probably shouldn't but that's a story for another post) or data members that have a complex relationship to one another, there could be a lot of benefit by having just one init function instead of two or more.
John Dibling
  • 99,718
  • 31
  • 186
  • 324
4

You're looking for Scott Meyers' Effective C++, Item 12: "Copy all parts of an object", whose summary states:

  • Copying functions should be sure to copy all of an object's data members and all of its base class parts.
  • Don't try to implement one of the copying functions in terms of the other. Instead, put common functionality in a third function that both call.
nnunes
  • 123
  • 1
  • 7
Billy ONeal
  • 104,103
  • 58
  • 317
  • 552