Swapping an object within itself

Question

I'm trying to swap an object within itself. It works but when I add a destructor it gives me a double free error. Is there a way to prevent this? The method I'm talking about is void swap(SimpleArray &object).

(Sorry if you read this before I had the wrong info in my post...)

#include "TestType.h"
class SimpleArray {

    private: 
        TestType* pArray;
        //TestType* temp;
    public:
        SimpleArray(TestType *array)
        {
            this->pArray = array;
        }
        ~SimpleArray() { delete[] pArray; }
        SimpleArray() { pArray = 0;}
        SimpleArray(const SimpleArray& arg){ pArray = arg.pArray; }
        ~SimpleArray() { delete[] pArray; }
        TestType * get() const{ return pArray; }
        bool isNonNull() const { return pArray != 0; }
        //TestType* pArray;
        void reset(TestType*& p) {this->pArray = p; }
        void reset() { pArray = 0; }

        void swap(SimpleArray &object) { SimpleArray temp; temp = object; object = *this; *this = temp;}
        TestType * release() { pArray = 0; return pArray; }
        TestType& getReference(int a) { return *pArray; }


};

This works but once I add the destructor it gives me a "double free or corruption error". How do I solve this? Here's the function in main where it messes up.

bool testGetReleaseSwap() {
    SimpleArray array1;
    if (array1.get() != 0)
        return false;

    TestType* directArray1 = new TestType[100];
    array1.reset(directArray1);
    if (array1.get() != directArray1)
        return false;

    TestType* directArray2 = new TestType[50];
    SimpleArray array2(directArray2);

    array1.swap(array2);
    if (array1.get() != directArray2 || array2.get() != directArray1)
        return false;

    array2.swap(array1);
    if (array1.get() != directArray1 || array2.get() != directArray2)
        return false;

    array1.swap(array1);
    if (array1.get() != directArray1)
        return false;

    if (array1.release() != directArray1 || array2.release() != directArray2)
        return false;

    if (array1.get() != 0 || array2.get() != 0)
        return false;

    delete[] directArray1;
    delete[] directArray2;

    return true;
}

Answer 1

The trivial way out here is to invoke temp.release() at the end if your swap method to prevent double deletion.

The underlying issue is much deeper, though. In C++ it is crucial to always maintain strict semantics of who owns something, for example a memory region that requires deletion.

A frequent pattern is that the object that allocates something is also responsible for cleaning up and no one else. This fits nicely with SimpleArray, but the copy constructor breaks it because it multiplies the number of owners!

To implement shared data semantics you have to invest more work (reference counting etc.) or you have to forbid array copying and make the copy constructor private.

A clean way to fix up swap to work without copying the object would be:

 void swap(SimpleArray &object) { 
    TestType* temp = object.pArray;
    object.pArray = this->pArray;
    this->pArray = temp;
 }

(std::swap(object.pArray, pArray); works as well)

Because to swap the memory regions of the array fits nicely with a single-owner pattern, what went wrong here is only the use of the full object copy.

You should read up on resource management and ownership semantics in C++. Your code will always be error prone unless you absolutely know who owns what.

Answer 2

It seems to me that you are trying to implement a class that has shallow copy semantics (and possibly copy-on-write). To do that successfully you need to track how many other owners of the shared data are still around and need to destroy the owned object, when that count reaches zero. You can either use a std::shared_ptr for that or implement the reference counting yourself.

As for the real problem in that specific example, look at what you copy constructor is doing. It is not copying but simply taking another reference (a pointer to be specific) to the object that is already owned by its argument. That by itself already enough to get a double free and your swap testcase is simply exposing that issue.