Assuming I have assignment operator, is my copy constructor being this = obj okay?

Question

Essentially, my question is as follows: assuming I've created an assignment operator for a class, is it against convention or frowned upon to have my copy constructor just be this = item?

Lets say I'm creating a templated class with only the following data:

private:
    int       _size;
    ItemType* _array;

If my assignment operator is as follows:

template<class ItemType>
void obj<ItemType>::operator = (const obj & copyThis){
    _size = copyThis.getSize(); 
    _array = new ItemType[_size];
    for (int i = 0; i < _size; i++){
        //assuming getItemAt is a function that returns whatever is in given location of object's _array
        _array[i] = copyThis.getItemAt(i);
    }
}

Then would it be against convention/looked down upon/considered incorrect if my copy constructor was simply as follows?

template<class ItemType>
obj<ItemType>::obj(const obj & copyThis){
    this = copyThis;
}

Your assignment operator will leak memory and doesn't handle self-assignment. — Michael Burr, Apr 21 '17 at 01:11
Your copy constructor is broken. It's leaking memory. So, you will end up with a mostly ok constructor, and a broken copy constructor that leaks memory, like a sieve. — Sam Varshavchik, Apr 21 '17 at 01:11
@DavidSchwartz as far as i know, 'this' refers to the object the member function belongs to, so 'this' is an obj — Ethan B., Apr 21 '17 at 01:14
@ethanbin: your `operator=` leaks the previous array. And if you call `operator=` from the copy constructor, you are not initializing your data members to default values before calling the operator. — Remy Lebeau, Apr 21 '17 at 01:22
@RemyLebeau thanks for the comment, and sorry if I'm missing something simple here, but how exactly does it leak the previous array, and how could I fix that? — Ethan B., Apr 21 '17 at 01:26
`operator=` copies data from an existing object to another existing object, so you have to `delete[]` the existing old array before replacing it. You are not doing that, so you are leaking the old array. — Remy Lebeau, Apr 21 '17 at 01:28
@RemyLebeau ahh! I see! I don't know how that did not occur to me. thanks! — Ethan B., Apr 21 '17 at 01:29
@ethanbin Your understanding of `this` is not correct. See section 9.3.2 of the C++ standard, "The type of `this` in a member function of a `class `X is `X*`." — David Schwartz, Apr 21 '17 at 01:40
@DavidSchwartz I did not know that, thanks for the correction! — Ethan B., Apr 21 '17 at 01:43

Remy Lebeau · Accepted Answer · 2017-04-21T01:52:48.833

It is generally safe to call operator= in the copy constructor (as long as the operator= does not try to use the copy constructor as part of its logic).

However, your operator= is implemented wrong to begin with. It leaks memory, does not handle this being assigned to itself, and does not return a reference to this.

Try this instead:

template<class ItemType>
obj<ItemType>::obj(const obj & copyThis)
    : _size(0), _array(0)
{
    *this = copyThis;
}

template<class ItemType>
obj<ItemType>& obj<ItemType>::operator=(const obj<ItemType> &copyThis)
{
    if (this != &copyThis)
    {
        int newSize = copyThis.getSize(); 
        ItemType *newArray = new ItemType[newSize];

        // consider using std::copy() instead:
        //
        // std::copy(copyThis._array, copyThis._array + newSize, newArray);
        //
        for (int i = 0; i < newSize; ++i) {
            newArray[i] = copyThis.getItemAt(i);
        }

        delete[] _array;
        _array = newArray;
        _size = newSize; 
    }

    return *this;
}

That being said, it is generally better to implement operator= using the copy constructor, not the other way around:

template<class ItemType>
obj<ItemType>::obj(const obj & copyThis)
    : _size(copyThis.getSize()), _array(new ItemType[_size])
{
    for (int i = 0; i < _size; ++i){
        _array[i] = copyThis.getItemAt(i);
    }

    // or:
    // std::copy(copyThis._array, copyThis._array + _size, _array);
}

template<class ItemType>
obj<ItemType>& obj<ItemType>::operator=(const obj<ItemType> &copyThis)
{
    if (this != &copyThis)
    {
        obj<ItemType> tmp(copyThis); 
        std::swap(_array, tmp._array);
        std::swap(_size, tmp._size);
    }

    return *this;
}

Which can be cleaned up a little if you add a swap method:

template<class ItemType>
obj<ItemType>::obj(const obj & copyThis)
    : _size(copyThis.getSize()), _array(new ItemType[_size])
{
    for (int i = 0; i < _size; ++i){
        _array[i] = copyThis.getItemAt(i);
    }
}

template<class ItemType>
void obj<ItemType>::swap(obj<ItemType> &swapThis)
{
    std::swap(_array, swapThis._array);
    std::swap(_size, swapThis._size);
}

template<class ItemType>
obj<ItemType>& obj<ItemType>::operator=(const obj<ItemType> &copyThis)
{
    if (this != &copyThis) {
        obj<ItemType>(copyThis).swap(*this); 
    }

    return *this;
}

That being said, if you replace your manual array with a std::vector, then you don't need to manually implement a copy constructor or a copy assignment operator at all, the compiler-generated default ones will suffice (since std::vector already implements copy semantics).

Ah, that explains the mistakes I made that others were commenting about extremely clearly, thanks! But, aside from calling `operator=` in a copy constructor being safe, is it against convention or seen as unusual? — Ethan B., Apr 21 '17 at 01:31
Correct me if I'm wrong, but isn't it usual to implement `operator=` as taking the argument by value, so you can use [the copy-and-swap idiom](http://stackoverflow.com/q/3279543/364696), make `operator=` `nothrow`, and generally dedup/simplify your code? — ShadowRanger, Apr 21 '17 at 01:40
@ShadowRanger: that is a common implementation, though it does have the side effect of always making a copy even for a self-assignment. As you can see in my answer, copy-and-swap is still an option even when the argument is passed by const reference, but at least self-assignment can avoid the copy. — Remy Lebeau, Apr 21 '17 at 01:46
@RemyLebeau: True. I usually consider it worth it for avoiding adding overhead to the common (non-self-assignment case) and particularly for working with rvalues efficiently (since it can avoid the copy operation entirely). — ShadowRanger, Apr 21 '17 at 01:50
@ShadowRanger: When working with rvalue references in C++11 and later, it does usually make more sense to pass the argument by value instead, and let the compiler decide whether to use copy or move sematics (provided you implement both correctly). For earlier C++ versions, it makes more sense to pass by const reference instead. — Remy Lebeau, Apr 21 '17 at 01:53

Assuming I have assignment operator, is my copy constructor being this = obj okay?

1 Answers1