Does delete[] work properly with generic arrays? If so why does using std::vector::erase on it cause error in freeing memory

Question

I was trying to work with arrays that are circular, and so ended up writing a CircularArray class for which I have attached the code. It uses a generic pointer for an array. When I try creating a list of such circular arrays using std::vector, I face a problem when I try to use erase on it.

I don't see why this should be the case as I think the destructors and copy constructor work well enough normally.

Can someone please help with this?

Code:
CircularArray Class

template<class T> class CircularArray
    {
        //Class denoted by 'T' is expected to have a functional assignment operator, i.e. operator=(const T& ext) {} in place
    protected:
        int size=0;
        int ori=0;
        T* array;
    private:
        int pos=0;

    public:
        CircularArray() : CircularArray(0) {}
        CircularArray(int s) {size=s;array=new T[s];}
        CircularArray(T* ptr,int s)// : CircularArray(s)
        {
            size=s;array=new T[s];
            for(int i=0;i<size;i++)
                array[i]=ptr[i];
        }
        CircularArray(const CircularArray<T>& arr) : CircularArray(arr.size)
        {
            for(int i=0;i<size;i++)
                array[i]=arr.array[i];
        }
        ~CircularArray() {delete[] array;}
        ...

Testing Code

int main()
{
    std::vector<CircularArray<int>> test;
    int *a1=new int[3] {1,2,3},*a2=new int[3] {1,2,3},*a3=new int[3] {1,2,3};
    CircularArray<int> n1(a1,3),n2(a2,3),n3(a3,3);
    test.push_back(n1);
    test.push_back(n2);
    test.push_back(n3);
    test.erase(test.begin()+1);
    for(auto v : test)
    {
        for(int i=0;i<3;i++)
            cout << v[i];
        cout << "\n";
    }
}

This program gives bad output after encountering the deleted part of the vector. Valgrind says that there is a memory corruption in trying to read freed memory. What is wrong?

Yachtsman

Missing assignment operator. Why don't you simply use a std::vector to hold your internal array instead of falling back down to manual memory management? – Pennoncel

Writing containers that directly use new/delete under the hood is often tricky. Avoid writing custom containers, and if you can't, at least avoid new/delete. – Kell

Thanks.. That solves it – Yachtsman

Just a side note... consider use smart-pointers and stl-containers, where possible. If I read new code and spy simple plain old c-pointers my stomach feels a little funny. – Resee

Answer 1

Vector elements must be copy/move assignable, yet you are relying on the default copy assignment operator which does not create any fresh memory. Your assigned objects all share the same memory space, later resulting in a double free.

Your constructors are good but you'll need a copy/move assignment operator too.

Read about the Rule of Five.

Also consider just using a std::vector for backing storage; it'll be much simpler.

Shindig

Answer 2

Does delete[] work properly with generic arrays?

Yes.

Your (implicitly generated) copy and move assignment operator are wrong. They will copy the member pointer. Then you have two pointers to the same array, and one destructor deletes it once, and another deletes it for a second time, which leads to undefined behaviour.

When manually managing dynamic resource, it is essential to keep track of ownership, and make sure that it is released exactly once. A typical solution is to use a smart pointer. Your class has unique ownership (or it would have, if it didn't accidentally share the ownership in the assignment operators) of the dynamic array, so a unique pointer would be an appropriate choice.

On the other hand, you could use a vector container instead of a smart pointer.

Muth