Using copy constructor in assignment operator

Question

Is it against style guidelines to use the copy-constructor in the assignment operator? I.e.:

const Obj & Obj::operator=(const Obj & source)
{
    if (this == &source)
    {
        return *this;
    }

    // deep copy using copy-constructor
    Obj * copy = new Obj(source);

    // deallocate memory
    this->~Obj();

    // modify object
    *this = *copy;

    return *copy;
}

Assuming that the copy-constructor performs a deep copy on the object.

---

EDIT:

My code is extremely erroneous, as commenters have pointed out.

As for the overall conceptual question: as WhozCraig suggested, the copy/swap idiom seems to be the way to go: What is the copy-and-swap idiom?

Answer 1

Here is the copy/swap idiom on your example in a nutshell:

#include <algorithm>

class Obj
{
    int *p;
    void swap(Obj& left, Obj& right);

public:
    Obj(int x = 0) : p(new int(x)) {}
    Obj(const Obj& s);
    Obj& operator = (const Obj& s);
    ~Obj() { delete p; }
};

Obj::Obj(const Obj& source) : p(new int(*source.p))
{}

void Obj::swap(Obj& left, Obj& right)
{
    std::swap(left.p, right.p);
}

Obj & Obj::operator=(const Obj & source)
{
    Obj temp(source);
    swap(*this, temp);
    return *this;
}

int main()
{
    Obj o1(5);
    Obj o2(o1);
    Obj o3(10);
    o1 = o3;
}

To see how it works, I purposefully created a member that is a pointer to dynamically allocated memory (this would be problematic if there were no user-defined copy constructor and assignment operator).

If you focus on the assignment operator, it calls the Obj copy constructor to construct a temporary object. Then the Obj-specific swap is called that swaps the individual members. Now, the magic is in the temp object after the swap is called.

When the destructor of temp is called, it will call delete on the pointer value that this used to have, but was swapped out with the temp pointer. So when temp goes out of scope, it cleaned up the memory allocated by the "old" pointer.

Also, note that during assignment, if new throws an exception during the creation of the temporary object, the assignment will throw an exception before any members of this become changed. This prevents the object from having members that may be corrupted due to having them changed inadvertently.

Now, a previous answer was given that uses the often-used "shared code" approach to copy assignment. Here is a full example of this method, and an explanation of why it has issues:

class Obj
{
    int *p;
    void CopyMe(const Obj& source);

public:
    Obj(int x = 0) : p(new int(x)) {}
    Obj(const Obj& s);
    Obj& operator = (const Obj& s);
    ~Obj() { delete p; }
};

void Obj::CopyMe(const Obj& source)
{
    delete p;
    p = new int(*source.p);
}

Obj::Obj(const Obj& source) : p(0)
{
   CopyMe(source);
}

Obj & Obj::operator=(const Obj & source)
{
    if ( this != &source )
        CopyMe(source);
    return *this;
} 

So you would say "what's wrong with this?" Well, the thing that's wrong is that CopyMe's first thing it does is call delete p;. Then the next question you'll ask is "So what? Isn't that what we're supposed to do, delete the old memory?"

The issue with this is that there is a potential for the subsequent call to new to fail. So what we did was destroy our data before we even know that the new data will be available. If new now throws an exception, we've messed up our object.

Yes, you can fix it easily by creating a temporary pointer, allocating, and at the end, assign the temp pointer to p. But many times this can be forgotten, and code such as above remains in the codebase forever, even though it has a potential corruption bug.

To illustrate, here is the fix for the CopyMe:

void Obj::CopyMe(const Obj& source)  
{
    int *pTemp = new int(*source.p);
    delete p;
    p = pTemp;
}

But again, you will see tons of code that use the "shared code" method that has this potential bug and not mention a word about the exception issue.

Answer 2

What you are trying to do doesn't work. You seem to be thinking that the object the assignment operator returns becomes the new "this", but that is not the case. You have not modified the object, just destroyed it.

Obj a;
Obj b;
a = b; // a has been destroyed
       // and you've leaked a new Obj.
// At the end of the scope, it will try to destroy `a` again, which
// is undefined behavior.

It is possible to implement an assignment operator by using placement new, but it is a fragile solution, and not generally recommended:

const Obj & Obj::operator=(const Obj & source)
{
    if (this == &source)
    {
        return *this;
    }

    this->~Obj();

    new (this) Obj(source);

    return *this;
}

This causes problems if exceptions are possible during construction and it potentially causes problems with derived classes.

Answer 3

It totally makes sense to share code between copy constructor and assigmnet operator because they often do the same operations (copying object passed as parameter attributes to this).

Personnaly, I often do it by smartly coding my assignment operator and then calling it from the copy constructor:

Obj::Obj(const Obj & source)
{
    Obj::operator=( source );
}

const Obj& Obj::operator=(const Obj& source)
{
    if (this != &source)
    {
        // copy source attribtes to this.
    }    
    return *this;
}

It works if you write the operator= correctly. As commented, one may recommend to have a swap function used by both: Copy constructor and = operator overload in C++: is a common function possible?

Anyway, your idea to share code between both functions was good, but the way you implemented it does not. work for sure..it has many problems and does not do what you mean to. It calls recursively the operator=. Moreover, you should never explictely call destructor functions as you did (this->~Obj();).