Is it bad form to call the default assignment operator from the copy constructor?

Question

Consider a class of which copies need to be made. The vast majority of the data elements in the copy must strictly reflect the original, however there are select few elements whose state is not to be preserved and need to be reinitialized.

Is it bad form to call a default assignment operator from the copy constructor?

The default assignment operator will behave well with Plain Old Data( int,double,char,short) as well user defined classes per their assignment operators. Pointers would need to be treated separately.

One drawback is that this method renders the assignment operator crippled since the extra reinitialization is not performed. It is also not possible to disable the use of the assignment operator thus opening up the option of the user to create a broken class by using the incomplete default assignment operator A obj1,obj2; obj2=obj1; /* Could result is an incorrectly initialized obj2 */ .

It would be good to relax the requirement that to a(orig.a),b(orig.b)... in addition to a(0),b(0) ... must be written. Needing to write all of the initialization twice creates two places for errors and if new variables (say double x,y,z) were to be added to the class, initialization code would need to correctly added in at least 2 places instead of 1.

Is there a better way?

Is there be a better way in C++0x?

class A {
  public:
    A(): a(0),b(0),c(0),d(0)
    A(const A & orig){
      *this = orig;       /* <----- is this "bad"? */
      c = int();
    }
  public:
    int a,b,c,d;
};

A X;
X.a = 123;
X.b = 456;
X.c = 789;
X.d = 987;

A Y(X);

printf("X: %d %d %d %d\n",X.a,X.b,X.c,X.d);
printf("Y: %d %d %d %d\n",Y.a,Y.b,Y.c,Y.d);

Output:

X: 123 456 789 987
Y: 123 456 0 987

Alternative Copy Constructor:

A(const A & orig):a(orig.a),b(orig.b),c(0),d(orig.d){}  /* <-- is this "better"? */

Answer 1

With your version of the copy constructor the members are first default-constructed and then assigned.
With integral types this doesn't matter, but if you had non-trivial members like std::strings this is unneccessary overhead.

Thus, yes, in general your alternative copy constructor is better, but if you only have integral types as members it doesn't really matter.

Answer 2

As brone points out, you're better off implementing assignment in terms of copy construction. I prefer an alternative idiom to his:

T& T::operator=(T t) {
    swap(*this, t);
    return *this;
}

It's a bit shorter, and can take advantage of some esoteric language features to improve performance. Like any good piece of C++ code, it also has some subtleties to watch for.

First, the t parameter is intentionally passed by value, so that the copy constructor will be called (most of the time) and we can modify is to our heart's content without affecting the original value. Using const T& would fail to compile, and T& would trigger some surprising behaviour by modifying the assigned-from value.

This technique also requires swap to be specialized for the type in a way that doesn't use the type's assignment operator (as std::swap does), or it will cause an infinite recursion. Be careful of any stray using std::swap or using namespace std, as they will pull std::swap into scope and cause problems if you didn't specialize swap for T. Overload resolution and ADL will ensure the correct version of swap is used if you have defined it.

There are a couple of ways to define swap for a type. The first method uses a swap member function to do the actual work and has a swap specialization that delegates to it, like so:

class T {
public:
    // ....
    void swap(T&) { ... }
};

void swap(T& a, T& b) { a.swap(b); }

This is pretty common in the standard library; std::vector, for example, has swapping implemented this way. If you have a swap member function you can just call it directly from the assignment operator and avoid any issues with function lookup.

Another way is to declare swap as a friend function and have it do all of the work:

class T {
    // ....
    friend void swap(T& a, T& b);
};

void swap(T& a, T& b) { ... }

I prefer the second one, as swap() usually isn't an integral part of the class' interface; it seems more appropriate as a free function. It's a matter of taste, however.

Having an optimized swap for a type is a common method of achieving some of the benefits of rvalue references in C++0x, so it's a good idea in general if the class can take advantage of it and you really need the performance.

Answer 3

I would call it bad form, not because you double-assign all your objects, but because in my experience it's often bad form to rely on the default copy constructor / assignment operator for a specific set of functionality. Since these are not in the source anywhere, it's hard to tell that the behavior you want depends on their behavior. For instance, what if someone in a year wants to add a vector of strings to your class? You no longer have the plain old datatypes, and it would be very hard for a maintainer to know that they were breaking things.

I think that, nice as DRY is, creating subtle un-specified requirements is much worse from a maintenance point of view. Even repeating yourself, as bad as that is, is less evil.

Answer 4

Personally I think the broken assignment operator is killer. I always say that people should read the documentation and not do anything it tells them not to, but even so it's just too easy to write an assignment without thinking about it, or use a template which requires the type to be assignable. There's a reason for the noncopyable idiom: if operator= isn't going to work, it's just too dangerous to leave it accessible.

If I remember rightly, C++0x will let you do this:

private:
    A &operator=(const A &) = default;

Then at least it's only the class itself which can use the broken default assignment operator, and you'd hope that in this restricted context it's easier to be careful.

Answer 5

I think the better way is not to implement a copy constructor if the behaviour is trivial (in your case it appears to be broken: at least assignment and copying should have similar semantics but your code suggests this won't be so - but then I suppose it is a contrived example). Code that is generated for you cannot be wrong.

If you need to implement those methods, most likely the class could do with a fast swap method and thus be able to reuse the copy constructor to implement the assignment operator.

If you for some reason need to provide a default shallow copy constructor, then C++0X has

 X(const X&) = default;

But I don't think there is an idiom for weird semantics. In this case using assignment instead of initialization is cheap (since leaving ints uninitialized doesn't cost anything), so you might just as well do it like this.

Answer 6

Essentially, what you are saying is that you have some members of your class which don't contribute to the identity of the class. As it currently stands you have this expressed by using the assignment operator to copy class members and then resetting those members which shouldn't be copied. This leaves you with an assignment operator that is inconsistent with the copy constructor.

Much better would be to use the copy and swap idiom, and express which members shouldn't be copied in the copy constructor. You still have one place where the "don't copy this member" behaviour is expressed, but now your assignment operator and copy constructor are consistent.

class A
{
public:

    A() : a(), b(), c(), d() {}

    A(const A& other)
        : a(other.a)
        , b(other.b)
        , c() // c isn't copied!
        , d(other.d)

    A& operator=(const A& other)
    {
        A tmp(other); // doesn't copy other.c
        swap(tmp);
        return *this;
    }

    void Swap(A& other)
    {
        using std::swap;
        swap(a, other.a);
        swap(b, other.b);
        swap(c, other.c); // see note
        swap(d, other.d);
    }

private:
    // ...
};

Note: in the swap member function, I have swapped the c member. For the purposes of use in the assignment operator this preserves the behaviour to match that of the copy constructor: it re-initializes the c member. If you leave the swap function public, or provide access to it through a swap free function you should make sure that this behaviour is suitable for other uses of swap.