How should I deal with the destructor, when making a custom vector with a template class?

Question

I tried to make my custom Vector class, with a template class.

I expect I can put my Vector<int> into a Vector<Vector<int>> variable. At least that was what I was hoping for... but it keeps crashing at the destructor code.

Here's my code.

#include <iostream>
#include <string>

template <typename T>
class Vector {

    T* data;
    int capacity;
    int length;

public:

    typedef T value_type;

    Vector() {}

    Vector(int n) : data(new T[n]), capacity(n), length(0) {}

    void push_back(T input) { 
        data[length++] = input;
    }

    T operator[](int i) { return data[i]; }

    virtual ~Vector() { if (data) delete[] data; }
};

int main() {
    Vector<Vector<int>> v(3);
    Vector<int> vv(4);
    v.push_back(vv);
}

So I thought, maybe I should use a copy constructor, since it seems the problem is that v is being deleted before vv. Well, if I just comment out the destructor code, it will work, but that doesn't seem right to me...

So I made a custom copy constructor like this:

Vector(const T& other) { 

}

But it give me an error, saying "ambiguous overloading"... looking back, of course it is wrong, since T of data is different from T of other...

How can I make my custom Vector class work? (i.e. I want push_back work as I intended...)

Answer 1

Your default constructor leaves the object completely uninitialized.

Consider what happens when you declare a

Vector<int> foo;

foo essentially gets a random memory address as data, a random length and capacity. This will give fireworks if you free it.

Perhaps you sidestepped this issue by always creating your vector with a predefined size. Luckily, trying to create/destroy a Vector<Vector<int>> brings this to light, because the Vector<int>[] inside your container still contains these ticking timebombs.

Answer 2

The general issue

In class design, especially when memory/resource allocation is involved, you typically need to follow the "Rule of Five" (which used to be the "Rule of Three" before C++11):

If you implement any of:

• A destructor
• An copy/move assignment operator
• A copy/move constructor

then you will probably need to implement all of them.

The reason is that each of them likely needs to have some resource management logic, beyond what the language gives you as a default.

The signatures of these five methods, for your class, would be:

Method	Signature
Copy constructor	Vector(const Vector&)
Move constructor	Vector(Vector&&)
Copy assignment operator	Vector& operator=(const Vector&)
Move assignment operator	Vector& operator=(Vector&&)
Destructor	~Vector() or virtual ~Vector()

Your specific class

In your specific case, there are several concrete problems:

• As @UnholySheep suggests, you mis-declared the copy constructor.
• You implemented a default constructor; but - it doesn't allocate anything, nor does it initialize anything! The data pointer holds arbitrary junk, and when you try to free it, bad things are likely to happen.
• You are performing quite a lot of copying of T values, which for the outer vector would be Vector<int> values - and that can get expensive.
• Even if you fixed the above, you should still implement the missing methods from the "rule of five".