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.

CodePudding user response：

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.

CodePudding user response：

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;
}