Trouble with destructor using

Question

I started learning c ++ and I ran into this problem. If I do not use the destructor, then everything works fine, but when I add it, the following error occurs:

*** Error in `./arraylist.o': double free or corruption (fasttop): 0xb71ab360 ***
./test_arraylist.sh: line 2:   513 Aborted                 ./arraylist.o

My source code:

#include <iostream>
#include "jppsys.h"

namespace jpparl {
    template <class T> class ArrayList {
        private:
            T *array;
            int step;
            long totalSize;
            long usedSize = 0;
        public:
            ArrayList(long step) {
                this->step = step;
                totalSize = step;
                array = new T[step];
                std::cout << "\nCreate " << array << "\n";
            }

            ArrayList() {
                this->step = 8;
                totalSize = step;
                array = new T[step];
                std::cout << "\nCreate " << array << "\n";
            }

            ArrayList(const ArrayList &arraylist) {
                step = arraylist.step;
                totalSize = arraylist.totalSize;
                usedSize = arraylist.usedSize;
                array = new T[totalSize];
                std::copy(arraylist.array, arraylist.array + totalSize, array);
                std::cout << "\nCopy " << array << "\n";
            }

            ~ArrayList() {
                std::cout << "\nDelete " << array << "\n";
                // delete[] array;
                // ^^^^^^^^^^^^^^^ error here
            }

            void add(T elem, long index) {
                if (usedSize == totalSize) totalSize += step;

                T *tmp = new T[totalSize];
                std::copy(array, array + index, tmp);
                std::copy(array + index, array + index + usedSize, tmp + index + 1);
                delete[] array;
                tmp[index] = elem;
                array = tmp;

                usedSize++;
            }

            void remove(long index) {
                if (usedSize == totalSize - step) totalSize -= step;
                T *tmp = new T[totalSize];
                std::copy(array, array + index, tmp);
                std::copy(array + index + 1, array + usedSize, tmp + index);
                delete[] array;
                array = tmp;

                usedSize--;
            }

            T get(long index) {
                return array[index];
            }

            long size() {
                return usedSize;
            }

            long getTotalSize() {
                return totalSize;
            }
    };

}

using namespace jpparl;
using namespace std;
int main() {
    ArrayList<ArrayList<int>> al(1);
    ArrayList<int> al2(1);
    cout << "\nAdding 256\n";
    al2.add(256,0); // al2.get(0) - 256
    cout << "\nAdding al2\n";
    al.add(al2, 0); // adds copy of a2, al.get(0) - copy of.a2
    cout << "\nRemoving 256\n";
    al2.remove(0); // removes 256 from al2, not from a1
    cout << al2.get(0) << " - must be 0      " << al.get(0).get(0) << "     - must be 256\n";
    cout << al2.size() << " - must be 0     " << al.get(0).size() << "     - must be 1\n";
}

And program outputs:

Without destructor:

Create 0xb86d4f30

Create 0xb86d4f18

Create 0xb86d5360

Adding 256

Adding al2

Copy 0xb86d5360

Create 0xb86d53a0

Delete 0xb86d4f30

Delete 0xb86d5360

Removing 256
0 - must be 0
Copy 0xb86d5370
256     - must be 256

Delete 0xb86d5370
0 - must be 0
Copy 0xb86d53d8
1      - must be 1

Delete 0xb86d53d8

Delete 0xb86d53c8

Delete 0xb86d5388

With destructor:

Create 0xb71aaf30

Create 0xb71aaf18

Create 0xb71ab360

Adding 256

Adding al2

Copy 0xb71ab360

Create 0xb71ab3a0

Delete 0xb71aaf30

Delete 0xb71ab360

Removing 256
0 - must be 0
Copy 0xb71ab370
0     - must be 256

Delete 0xb71ab370
0 - must be 0
Copy 0xb71ab370
1     - must be 1

Delete 0xb71ab370

Delete 0xb71ab360

Delete 0xb71ab388

Delete 0xb71ab360
*** Error in `./arraylist.o': double free or corruption (fasttop): 0xb71ab360 ***
./test_arraylist.sh: line 2:   513 Aborted                 ./arraylist.o

I will be grateful for help

Answer 1

Your class does not follow the Rule of 3, which basically states that if a class needs to implement a copy constructor, a copy assignment operator, or a destructor, it likely needs to implement all three (C++11 introduces move semantics by adding a move constructor and move assignment operator as well, thus making the Rule of 5).

Your class has a copy constructor and a destructor, but it is missing a copy assignment operator, which gets called by std::copy() when copying elements that are themselves instances of your class. Your class is also missing a move constructor and move assignment operator, as you are using C++11 (by virtual of how you are initializing your usedSize member).

Also, your add() and remove() methods are not managing the array correctly, either. There is no need to reallocate the array on every add/remove operation. That defeats the whole purpose of your step member. Reallocate only when actually needed (growing/shrinking beyond the current step).

The best option in this situation is to simply use std::vector instead of a manual array, eg:

#include <vector>
#include <utility>
#include "jppsys.h"

namespace jpparl {
    template <class T>
    class ArrayList {
    private:
        std::vector<T> array;

    public:
        void add(const T &elem) {
            array.push_back(elem);
        }

        void add(const T &elem, long index) {
            array.insert(array.begin()+index, std::move(elem));
        }

        void remove(long index) {
            array.erase(array.begin()+index);
        }

        T get(long index) {
            return array[index];
        }

        long size() {
            return array.size();
        }

        long getTotalSize() {
            return array.capacity();
        }
    };
}

If that is not an option for you, if you must use a manual array, then you should utilize the copy-swap idiom when making copies of the array, eg:

#include <algorithm>
#include <utility>
#include "jppsys.h"

namespace jpparl {
    template <class T>
    class ArrayList {
    private:
        T *array;
        int step;
        long totalSize;
        long usedSize = 0;

    public:
        ArrayList(long step = 8) {
            this->step = step;
            totalSize = step;
            array = new T[totalSize];
        }

        ArrayList(const ArrayList &src) {
            step = src.step;
            totalSize = src.totalSize;
            usedSize = src.usedSize;
            array = new T[totalSize];
            std::copy(src.array, src.array + usedSize, array);
        }

        ArrayList(ArrayList &&src) {
            step = src.step;
            totalSize = 0;
            usedSize = 0;
            array = nullptr;
            src.swap(*this);
        }

        ~ArrayList() {
            delete[] array;
        }

        ArrayList& operator=(const ArrayList &rhs) {
            if (&rhs != this)
                ArrayList(rhs).swap(*this);
            return *this;
        }

        ArrayList& operator=(ArrayList &&rhs) {
            rhs.swap(*this);
            return *this;
        }

        void swap(ArrayList &other) {
            std::swap(step, other.step);
            std::swap(totalSize, other.totalSize);
            std::swap(usedSize, other.usedSize);
            std::swap(array, other.array);
        }

        void add(const T &elem, long index = -1) {
            if (index == -1) index = usedSize;
            if (usedSize == totalSize) {
                ArrayList tmp(totalSize + step);
                std::copy(array, array + usedSize, tmp.array);
                std::swap(array, tmp.array);
                totalSize = tmp.totalSize;
            }
            std::copy_backward(array + index, array + index + (usedSize - index), array + usedSize + 1);
            array[index] = elem;
            ++usedSize;
        }

        void remove(long index) {
            std::copy(array + index + 1, array + usedSize, array + index);
            --usedSize;
            if ((usedSize == (totalSize - step)) && (totalSize > step)) {
                ArrayList tmp(totalSize - step);
                std::copy(array, array + usedSize, tmp.array);
                std::swap(array, tmp.array);
                totalSize = tmp.totalSize;
            }
        }

        T get(long index) {
            return array[index];
        }

        long size() {
            return usedSize;
        }

        long getTotalSize() {
            return totalSize;
        }
    };
}

Answer 2

The asignment operator is missing. In the line

al.add(al2, 0);

al2 Is shallow copied, meaning that the two objects share the same buffer. It then gets deleted twice. Once after the temporary that was passed to the add method is destroyed and once at the end of the main function.

Answer 3

As Neil Butterworth said, I needed to add an assignment operator.

#include <iostream>

namespace jpparl {
    template <class T> class ArrayList {
        private:
            T *array;
            int step;
            long totalSize;
            long usedSize = 0;

            void copy(const ArrayList &arraylist) {
                step = arraylist.step;
                totalSize = arraylist.totalSize;
                usedSize = arraylist.usedSize;
                array = new T[totalSize];
                std::copy(arraylist.array, arraylist.array + totalSize, array);
                std::cout << "\nCopy " << array << "\n";
            }

        public:
            ArrayList(long step) {
                this->step = step;
                totalSize = step;
                array = new T[step];
                std::cout << "\nCreate " << array << "\n";
            }

            ArrayList() {
                this->step = 8;
                totalSize = step;
                array = new T[step];
                std::cout << "\nCreate " << array << "\n";
            }

            ArrayList(const ArrayList &arraylist) {
                copy(arraylist);
            }

            ~ArrayList() {
                std::cout << "\nDelete " << array << "\n";
                delete[] array;
            }

            ArrayList& operator = (const ArrayList &arraylist) {
                copy(arraylist);
            }

            void add(T elem, long index) {
                if (usedSize == totalSize) totalSize += step;

                T *tmp = new T[totalSize];
                std::copy(array, array + index, tmp);
                std::copy(array + index, array + index + usedSize, tmp + index + 1);
                delete[] array;
                tmp[index] = elem;
                array = tmp;

                usedSize++;
            }

            void remove(long index) {
                if (usedSize == totalSize - step) totalSize -= step;
                T *tmp = new T[totalSize];
                std::copy(array, array + index, tmp);
                std::copy(array + index + 1, array + usedSize, tmp + index);
                delete[] array;
                array = tmp;

                usedSize--;
            }

            T get(long index) {
                return array[index];
            }

            long size() {
                return usedSize;
            }

            long getTotalSize() {
                return totalSize;
            }
    };

}

using namespace jpparl;
using namespace std;
int main() {
    ArrayList<ArrayList<int>> al(1);
    ArrayList<int> al2(1);
    cout << "\nAdding 256\n";
    al2.add(256,0); // al2.get(0) - 256
    cout << "\nAdding al2\n";
    al.add(al2, 0); // adds copy of a2, al.get(0) - copy of.a2
    cout << "\nRemoving 256\n";
    al2.remove(0); // removes 256 from al2, not from a1
    cout << al2.get(0) << " - must be 0      " << al.get(0).get(0) << "     - must be 256\n";
    cout << al2.size() << " - must be 0     " << al.get(0).size() << "     - must be 1\n";
}