Is this a valid copy constructor?

Question

I've just started learning about Rule of Three and was wondering if the following approach was sufficient for a copy constructor:

Array<T, ROW, COL>(const Array<T, ROW, COL> &array) {
    *this = array;
}

If it isn't sufficient, why not?

EDIT Added the assignment operator per request

inline Array<T, ROW, COL>& operator=(const Array<T, ROW, COL> &other) {;
    for (int i = 0; i < ROW; ++i) {
        for (int j = 0; j < COL; ++j) {
            data[j*ROW + i] = other(i, j);
        }
    }

    return *this;
}

Answer 1

General notes (before you edited your question):

Just calling the copy assignment operator in a copy constructor is not a general solution. They mean different things (I mean, why should they be there if they meant the same).

• The assignment operator is called when an (existing!) object is assigned a new value. If this object is of the same type, we call this also the copy assignment, since a typical implementation simply copies the contents (but might share some referenced things, like for example shared pointers or PIMPL classes with shared data). A copy assignment operator is implemented by the compiler automatically unless you provide one. It copies each member using the assignment operators of their types (primitive members are also copied).

• The copy constructor is called when a (not yet existing!) obejct is assigned an initial value of the same type, i.e. it should be initialized with the copy of an existing object. Again, if you don't provide a copy constructor, the compiler generates one for you, again just copying the members using the copy constructor.

If you call the assignment operator from within the copy constructor, this means that the generated program performs the following steps when copy-initializing a new object:

• (Unless you use member-initializer list:) It initializes non-primitive class members with default constructors. Primitive types are left uninitialized.
• Then, the assignment operator is called. If you didn't define one, it copies all members.

So it should be fine in most cases, but there are a couple of cases in which this does not work, in particular if your class has members which can't be default-constructed or can't be assigned. If this is the case, and if they can still be copy-constructed (in contrast to copy-assigned), you have to initialize the members in the member-initialization list of the copy constructor manually.

---

EDIT (Since the question got edited): In your case, data is a primitive type (all pointers are considered primitive), so you have to initialize it properly in the copy constructor before calling your assignment operator. If you don't do so, the assignment will delete an uninitialized pointer. So the best you should to (to avoid code duplication; it could be more efficient if you did):

Array<T, ROW, COL>(const Array<T, ROW, COL> &array) :
    data(0)  // Now it is at least initialized, although inefficient
{
    *this = array;
}

The assignment operator will then try to delete a null-pointer, which is okay (it just does nothing), and then performs the actual copy. Consider the data(0) initialization just as a "default-constructed null Array<...>" object, just temporarily. (Maybe you already provide a null-object which does not allocate external memory?)

Answer 2

There is one important reason why it's not enough with the code you proposed:

• The assignment operator is applied on a initialized object, but this is not the case in the copy constructor. Specifically, your data member is not initialized properly when the code runs *this = array (data isn't in a member-initializer list), so you are calling delete[] operator on the uninitialized data.

It requires you to, at least, use a member-initializer list to initialize the data member before you use it.

There are more drawbacks about your propose:

• The assignment operator forces restrictions on T type (but maybe that restrictions are in the class Array anyway):
  • T must be default-constructible (as noted by leemes).
• assignment operator is not good implemented:
  • is not considering case a = a (self-assignment), as Nick Bougalis noted.
• is not exception safe:
  • if new[] operator fails, the object will be in an inconsistent state.
  • if copying T throws an exception, the object will be in an inconsistent state.

I see a much better approach with using the copy constructor in assignment operator implementation instead of the opposite:

Array<T, ROW, COL>(const Array<T, ROW, COL> &array): data(new T[ROW * COL]) {
    for (int i = 0; i < ROW; ++i) {
        for (int j = 0; j < COL; ++j) {
            data[j*ROW + i] = other(i, j);
        }
    }
}

Array<T, ROW, COL>& operator=(Array<T, ROW, COL> other) {
    swap(other);
    return *this;
}

void swap(Array<T, ROW, COL>& other) {
    T* tmp = data;
    data = other.data;
    other.data = tmp;
}

(This is also known as the copy-and-swap idiom.)

Answer 3

In addition to the excellent post by leemes, let me add one note of caution. In your operator= implementation you should consider adding a check to at least handle self-assignment (that is a = a;). Also, if your data member variable is a pointer (as your original code suggests), then you MUST make sure that it points to some valid memory, because if you initialize it to point to 0 in the copy-constructor your assignment operator will crash when it tries to dereference it.

inline Array<T, ROW, COL>& operator=(const Array<T, ROW, COL> &other) 
{
    if(this == &other)
        return *this; // nothing to do!

    /* You should make sure that either data already points to an allocated
     * buffer of the appropriate size, or you should allocate it at this point.
     */

    assert(data != NULL);

    for (int i = 0; i < ROW; ++i) {
        for (int j = 0; j < COL; ++j) {
            data[j*ROW + i] = other(i, j);
        }
    }

    return *this;
}