c++ - Destructor called on assignment

Question

I'm still learning the basics of c++ so I may not have had the correct vocabulary to find the answer to my question but I couldn't find this mentioned anywhere.

If I have a class with a constructor and destructor why does the destructor get called on the new data when I am assigning to the class?

For example:

#include <iostream>

class TestClass {
    public:
    int* some_data;

    TestClass() {
        std::cout << "Creating" << std::endl;
        some_data = (int*)malloc(10*sizeof(int));
    }

    ~TestClass() {
        std::cout << "Deconstructing" << std::endl;
        free(some_data);
    }

    TestClass(const TestClass& t) : some_data{t.some_data} {
        std::cout << "Copy" << std::endl;
    }
};

int main() {
    TestClass foo;
    std::cout << "Created once" << std::endl;
    foo = TestClass();
    std::cout << "Created twice" << std::endl;
}

which prints:

Creating
Created once
Creating
Deconstructing
Created twice
Deconstructing
free(): double free detected in tcache 2
Aborted (core dumped)

So after following this in the debugger it appears the deconstructor is called on the newly created data which is confusing to me. Shouldn't the original data be freed once and then at the end of execution the new data should be freed? It seems like the original data is never freed like this.

and another very important piece of vocabulary: [rule of 3/5](https://stackoverflow.com/questions/4172722/what-is-the-rule-of-three) — 463035818_is_not_an_ai, Sep 30 '21 at 16:57
Your copy constructor is broken, you end up with 2 objects that have a pointer to the same `some_data`. Also you are missing an assignment operator overload (which is what `foo = TestClass();` calls) — UnholySheep, Sep 30 '21 at 16:57

Chris Uzdavinis · Accepted Answer · 2021-09-30T17:05:30.497

Your object owns a raw pointer to allocated memory, but does not implement a proper copy constructor that makes an allocation and copies the data behind the pointer. As written, when you copy an object, the pointer is copied, such that now two objects point to the same address (and the old one that the just-assigned-to object is leaked.)

When the temporary goes out of scope, it deletes its pointer but the copy (foo) still points to it. When foo goes out of scope, it deletes the same pointer again, causing this double free error you're seeing.

If you need to write a destructor to clean up, you almost always need to also provide copy and assignment operations, or disable them.

SUGGESTIONS:

hold the pointer in a std::unique_ptr which will fail to compile if you try to copy it. This forces you to deal with the issue. Also, malloc and free are mainly for C or low-level C++ memory management. Consider using new and delete for allocations instead. (unique_ptr uses delete by default, not free, and you must not mix them.)
alternately, delete the copy constructor and assignment operator
also, consider when you want to move from an xvalue (temporary or moved lvalue), you can pilfer the allocation from the right-hand-side. So this class is a good candidate for move constructor and move assignment.

not only assignment is missing, op does have a copy constructor, but it is not doing the right thing — 463035818_is_not_an_ai, Sep 30 '21 at 17:00
I never checked the copy constructor because it is not called, the output is never printing from it. But the assignment operator problem sounds correct — Matthew, Sep 30 '21 at 17:03

score 0 · Answer 2 · answered Sep 30 '21 at 17:56

Most of the comments and a few details more in code:

#include <iostream>
#include <array>
#include <memory>

class TestClass 
{
// members of a class should not be public
private:

    // TestClass owns the data, this is best modeled 
    // with a unique_ptr. std::array is a nicer way of
    // working with arrays as objects (with size!)
    std::unique_ptr<std::array<int, 10>> some_data;

public:
    TestClass() :
        some_data{ std::make_unique<std::array<int,10>>() }
    {
        std::cout << "Creating" << std::endl;

        // set everything in the array to 0
        std::fill(some_data->begin(), some_data->end(), 0);
    }

    ~TestClass() 
    {
        std::cout << "Destructing" << std::endl;
        // no need to manually delete a std::unique_ptr
        // its destructor will free the memory
        // and that will be called as part of this destructor
    }

    TestClass(const TestClass& t) : 
        // when you copy a class the copy should have its
        // own copy of the data (to avoid deleting some data twice)
        // or you must chose shared ownership (lookup std::shared_ptr)
        some_data{ std::make_unique<std::array<int,10>>() }
    {
        std::cout << "Copy" << std::endl;

        // copy data from t to this instances array
        // (note this would not have been necessary
        // it your array was just a non-pointer member,
        // try that for yourself too.)
        std::copy(some_data->begin(), some_data->end(), t.some_data->begin());
    }

    TestClass(TestClass&& rhs) :
        some_data{ std::move(rhs.some_data) } // transfer ownership of the unique_pointer to this copy
    {
        std::cout << "Move" << std::endl;
    }

    // Important this assignement operator is used in your original code too
    // but you couldn't see it!
    TestClass& operator=(const TestClass& t)
    {
        some_data = std::make_unique<std::array<int, 10>>();
        std::copy(some_data->begin(), some_data->end(), t.some_data->begin());

        std::cout << "Assignment" << std::endl;
        return *this;
    }

    

};

int main() 
{
    TestClass foo;
    std::cout << "Created once" << std::endl;

    foo = TestClass();
    std::cout << "Created twice" << std::endl;

    TestClass bar{ std::move(foo) };
    std::cout << "Moved" << std::endl;
}

c++ - Destructor called on assignment

2 Answers2