2

I'm having trouble understanding some basic memory management principles in C++. This code is part of a loop that is part of a function that reads in a maze file into a 2D vector.

According to Valgrind, the following code is causing memory leaks...

Note that t is a MazeNode object and verts is a vector within the t class that holds pointers to node objects (not to be confused with MazeNode objects):

node* top = new node(TOP, rowCount, i, t.type); node* bot = new node(BOTTOM, rowCount, i, t.type); node* left = new node(LEFT, rowCount, i, t.type); node* right = new node(RIGHT, rowCount, i, t.type); t.verts.push_back(top); t.verts.push_back(bot); t.verts.push_back(left); t.verts.push_back(right); temp.push_back(t); top = NULL; bot = NULL; left = NULL; right = NULL; delete top; delete bot; delete left; delete right;

Initially I did not set each of the pointers to NULL before deleting them, but would get allocation errors. So I just set them to NULL and my code works. I guess I'm just really confused why this would cause memory leaks and why I would need to set the pointers to NULL. There is probably a way easier non-pointer way to do this, but maybe this problem will help me understand memory management better.

Thanks everyone.

EDIT: Here's the MazeNode class (which is what 't' is) (also excuse my lazyness in writing this class, making everythign public like a struct)

class MazeNode { public: void setType(char c); char getChar(); NodeType type; vector<Direction> visitedFrom; vector<node*> verts; };

And the node class:

class node { public: node(); node(Direction d, int r, int c, NodeType t); ~node(); //empty definition node(const node* n); node& operator=(const node& n); void addAdj(node* a, int w); void printAdj() const; string direction() const; void print() const; bool operator<(const node& n) const; int distance; //from start bool visited; node* prev; vector<Edge> adj; Direction dir; int row, col; NodeType type; };

EDIT2: Thanks everyone. I understand the problem now. I changed my vectors of pointer objects so that I wasn't using pointers anymore.

Improve this question
8
  • You don't need to set the pointers to NULL. That seems to be exactly what is causing a leak. Commented Apr 16, 2012 at 21:57
  • Please add the definition of t.verts. Commented Apr 16, 2012 at 21:57
  • That's what I thought, but then why am I getting errors if I just use delete? Commented Apr 16, 2012 at 21:58
  • 1
    Please post the definition of your t and temp variables. I'm pretty sure that it's more than just setting to NULL that's causing the leak, but I need variable definitions to show what's "all" wrong here. Commented Apr 16, 2012 at 21:58
  • 2
    Don't use new, don't use delete. Those features of C++ are not suited for most code. Commented Apr 16, 2012 at 21:58

6 Answers 6

Reset to default
9

Prior to adding the null assignment, your code had a problem different (worse) than a memory leak: storing and probably also using a stray pointer, that is, a pointer pointing into de-allocated memory.

Making it a memory leak by adding the null assignment makes it better, but not much.

The real solution is not to keep any pointers anywhere after you have called delete on them. That is, do not push_back or do not delete here.

Improve this answer
Sign up to request clarification or add additional context in comments.

4 Comments

"The real solution is not to keep any pointers anywhere after you have called delete on them." .... Well, depends. That may be a nice rule of thumb for simple scenarios, but for complex ones you'll end up having arrays of pointers that you reuse and such. You may want to make your wording a bit more lenient as to not be absolute where there is no absolute.
Sorry I'm kind of new at this, but how can I have a vector of pointers then, without pushing back? Should I just not use a vector of pointer objects?
@Kaganar - It's certainly simplification, but a practical one while looking at a SIGABORT for maybe the first time.
@Slims - You can definitely have a vector of pointers. However, I suspect that either t or or temp in your example are not shown in their full scope. If either of these variables (or any copy of either) survives and is used past the delete, you are walking on a minefield. The pointers become dangerous to dereference. Post more code perhaps to show what you do with them, or to which library you ever pass them.
4

You are placing the pointers into a container, then deleting the pointers. When your code later tries to use those pointers they are invalid and cause a crash.

By setting the pointers to NULL before you delete them, you end up not deleting them at all - deleting a NULL pointer doesn't do anything. But now there's nothing to delete the objects later, and you get a memory leak.

You need to find some spot in the code where you're not using the pointers anymore, and delete them there.

Edit: Of course I should have mentioned that a smart pointer such as std::shared_ptr eliminates this hassle altogether, because it deletes the object automatically.

Improve this answer

Comments

2

You are setting the values to NULL before deleting them, so you are trying to delete NULL and nothing is being deleted. Try moving the delete calls above the NULL calls.

Improve this answer

Comments

2

This confusion is exactly why I create a macro for these kind of things:

#define delobj(obj) (delete obj, obj = NULL)

And you would use it like this:

delobj(top); delobj(bot); delobj(left); delobj(right);
Improve this answer

Comments

2

The error is using the vector of pointers. According to you, verts is this:

vector<node*> verts;

But what it should be is this:

vector<node> verts;

In the first case, when you push_back() the pointer, that's OK, but when you pop_back or otherwise re-size the vector, the pointer is the "contents" of the vector, and is de-allocated, but not what the pointer points to, which is the node. Hence the node leaks. But in the second case, the node is "part" of the vector, and is allocated/deallocated as part of re-sizing the vector.

Your pattern here probably indicates a Java/C# background, as "new-ing" into a container is very very common in those languages, but to do that in C++, you need a container of smart pointers (like vector<shared_ptr<node>> or something), which is probably beyond the scope of the question. But in those languages, every reference to a reference type is a "smart pointer" (more or less) and so this is done automatically. C++ isn't like that.

You either need to change your code to use a vector<node> (and change how you're pushing back on to it) or you need to explicitly de-allocate your nodes when the vector shrinks.

Improve this answer

1 Comment

I see. Well the vector next shrinks. It is used for the entire program; it's just a structure to hold the maze. It should never shrink.
0

Change to:

delete top; delete bot; delete left; delete right; top = NULL; bot = NULL; left = NULL; right = NULL;

And it should work.

Improve this answer

Comments

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.