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...)

Unless you initialize `data`, the value of this member variable can be arbitrary. Unless this value happens to be null, calling `delete[]` for it results in undefined behavour which crashes your program in this case. (Your default constructor doesn't initialize it to null.) Btw: `delete[]` can be called with null as operand; in this case it simply does nothing. The check for `data` being null in the destructor is unnecessary. — fabian, Jun 10 '22 at 08:43
Note: If you want to make `v[0].push_back(1);` work for `Vector>`, you should a reference from the `[]` operator: `T& operator[](int i) { ... }` Furthermore I recommend using an unsigned integral type for the index. Usually `size_t` is used for this purpose which would also be the preferred type for `capacity`/`length`. — fabian, Jun 10 '22 at 08:51
Implementing vector properly is not trivial as it must involve placement `new`. — Quimby, Jun 10 '22 at 08:56
Your class constructors explicitly do dynamic memory allocation. You need to ensure that all constructors - including copy/move constructors - allocate memory when needed, and that assignment (copy/move) operators correctly reallocate memory when needed, otherwise copying an instance of your `Vector` will cause the destructor to release some memory twice - which causes undefined behaviour. The implicitly generated defaults for those functions DO NOT do that. For more information look up "rule of three" or (C++11 and later) "rule of five". — Peter, Jun 10 '22 at 09:26
@Quimby implementing exactly `std::vector` is tricky, but something with *similar* requirements is plausible — Caleth, Jun 10 '22 at 10:14
@Caleth When you want to store non-trivial values, as OP wanted to store nested vectors, you have to have use placement `new` or reallocate the whole array on each push back, that is not good. — Quimby, Jun 10 '22 at 13:06

einpoklum · Answer 1 · 2022-06-10T13:06:55.060

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".

score 5 · Answer 2 · answered Jun 10 '22 at 08:45

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.

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

2 Answers2

The general issue

Your specific class