erasing element from std::vector

Question

Can we expect the following code work always without giving any error?

int a=1, b=1, c=1;

std::vector <int*> edges;
edges.resize(3);

edges[0] = new int;
edges[0] = &a;

edges[1] = new int;
edges[1] = &b;

edges[2] = new int;
edges[2] = &c;

delete edges[0];
edges[0] = NULL;
edges.erase(edges.begin());

delete edges[0];
edges[0] = NULL;
edges.erase(edges.begin());

delete edges[0];
edges[0] = NULL;
edges.erase(edges.begin());

No, it is totally broken. You are leaking memory, and calling `delete` on things that shouldn't be deleted. I would not expect it to ever work, but undefined behaviour can sometimes look like it is working. — juanchopanza, Apr 02 '14 at 21:43
@juanchopanza: Then how should I erase a specific pointer from the container? — Shibli, Apr 02 '14 at 21:47
Erasing is the least of your worries. It is worth taking a step back and figuring out what pointers are, and what dynamic allocation is. There are plenty of questions on this, and introducory C++ books *might* cover it. Introductory C books should definitely cover it. — juanchopanza, Apr 02 '14 at 21:49

Max Lybbert · Accepted Answer · 2014-04-02T22:11:20.977

I think I see where you're going, but you've fallen into a common trap for people who learned Java or C# before C++:

int a=1, b=1, c=1;

std::vector <int*> edges;
edges.resize(3);

edges[0] = new int;  // this line is unnecessary
edges[0] = &a;

The call to new int allocates memory for an int, and stores the address in edges[0]. But the very next line decides to point edges[0] to the address of a. The memory allocated by new int is leaked.

What's more, the call to delete edges[0] is effectively a call to delete &a, which is not what you want at all (you can only delete nullptr or things you got from new; and you did not get a from new).

Unlike Java or C#, new actually means something in C++.

The calls to edges.erase(edges.begin()) are all good, though.

So, edited, this would work:

std::vector <int*> edges;
edges.resize(3);

edges[0] = new int(1);
...
delete edges[0];
edges.erase(edges.begin());

But I have to wonder why you aren't using std::vector<std::unique_ptr<int>>, which will take care of the memory management for you:

std::vector<std::unique_ptr<int>> edges;
edges.resize(3);
edges[0] = std::unique_ptr<int>(new int(1)); // or edges[0] = std::make_unique(1); in C++14
...

edges.erase(edges.begin());
...

In fact, if int isn't a stand-in for some other type (i.e., you actually want a container of ints), you should just use std::vector<int>:

std::vector<int> edges;
edges.resize(3);
edges[0] = 1;
edges[1] = 2;
edges[2] = 3;

edges.erase(edges.begin());
edges.erase(edges.begin());
edges.erase(edges.begin());

I like "`new` actually means something in C++". It reminded me of [a question I once asked](http://stackoverflow.com/questions/6340535/is-the-new-keyword-in-java-redundant). — juanchopanza, Apr 02 '14 at 21:50

erasing element from std::vector

1 Answers1