CODEWITHSUNDEEP
c++operator-overloadingdestructor
MartinYakuza
MartinYakuza

Reputation: 135

C++assingment operator using destructor and copying constructor

I was working on a class with multiple dynamic fields and I was looking for quick way of coding assignment operator.

So let's say I have some basic class Cla, which stores dynamic array of integers (arr) and the size of said array (n).

I've coded this:

Cla::Cla(int* arr, int n) : n(n)
{
    this->arr = new int[n];

    //allocation error handling

    while (--n >= 0)
        this->arr[n] = arr[n];
}


Cla::~Cla()
{
    delete[] arr;
}

Cla::Cla(const Cla& copy) : Cla(copy.arr, copy.n){}

Cla& Cla::operator= (const Cla& asg)
{
    this->~Cla();
    *this = asg;
    return *this;
}

All of it works properly, except for operator=. The idea was that I'll just destroy my object and then create it again with copying constructor (for the sake of simplicity of the example I don't consider the situation where both objects have the same size and there is no need for deallocation and new allocation). It compiles, but it gives me some nasty errors when executed.

Can you give me some advice on how to correct this code? Is this even possible for it to work this way? (I know how to write an assingment operator, I'm just asking whether it is possible to do it using destructor and copying constructor. I couldn't find anything like that on the internet.)

Upvotes: 0

Views: 71

Answers (1)

Remy Lebeau
Remy Lebeau

Reputation: 595467

Your operator= has undefined behavior. First, you cannot manually call a destructor on an object that was not allocated with placement-new. Second, once an object is destroyed, it cannot be used anymore, which means *this = asg is accessing invalid memory once this->~Cla() has been called, as this is no longer pointing at a valid object. Third, your operator= is running an endless recursion loop, calling itself over and over until the call stack blows up (if you are lucky).

Since you want to use your copy constructor, your operator= would be better served by using the copy-swap idiom instead. Construct a local object to make use of your copy constructor, and then swap the contents of that object with this so that this takes ownership of the copied data and the local object frees the old data when it goes out of scope, eg:

Cla& Cla::operator= (const Cla& asg)
{
    if (&asg != this)
    {
        Cla temp(asg);
        std::swap(arr, temp.arr);
        std::swap(n, temp.n);
    }
    return *this;
}

Alternatively:

void Cla::swap(Cla &other)
{
    std::swap(arr, other.arr);
    std::swap(n, other.n);
}

Cla& Cla::operator= (const Cla& asg)
{
    if (&asg != this) {
        Cla(asg).swap(*this);
    }
    return *this;
}

That being said, the fact that your copy constructor is delegating to your converting constructor means that you are using C++11 or later, in which case you should also implement move semantics into your class, not just copy semantics, eg:

Cla::Cla() : arr(nullptr), n(0)
{
}

Cla::Cla(int* arr, int n) : arr(new int[n]), n(n)
{
    while (--n >= 0)
        this->arr[n] = arr[n];
}

Cla::Cla(Cla &&c) : arr(nullptr), n(0)
{
    c.swap(*this);
}

Cla::Cla(const Cla& c) : Cla(c.arr, c.n)
{
}

Cla::~Cla()
{
    delete[] arr;
}

void Cla::swap(Cla &other)
{
    std::swap(arr, other.arr);
    std::swap(n, other.n);
}

Cla& Cla::operator= (Cla asg)
{
    asg.swap(*this);
    return *this;
}

By passing the asg parameter by value, operator= can decide whether to use copy semantics or move semantics at the call site based on whether an lvalue or rvalue is being passed to it. The compiler will pick the appropriate constructor to construct the asg parameter with, and then this can take ownership of the resulting data.

Upvotes: 1

Related Questions