4

In my simple game engine I have a vector containing pointers to entities. Each entity is allocated with the new keyword. To call functions on my entities I use an iterator and so when deleting my entities I figured I'd use an iterator as well, like so:

vector<Entity*>::iterator iter;
for (iter = gameEntitys.begin(); iter != gameEntitys.end();) {
    delete (*iter);
    ++iter;
}

But I now have a horrific memory leak and all profiling tools point towards the line delete (*iter);

Clearly there is something wrong but what is the correct way for me to delete the entities contained in the vector (and clear the vector for another scene). Using gameEntitys.clear(); is useless to my knowledge as it only removes the elements of the vector not actually calling delete on them as they are pointers rather than actual data.

Edit: I have bee watching the comments. The Entity class is not polymorphic and does not have any subclasses. Would it make more sense if I stopped using dynamic memory and had a normal array of non-pointer entities?

The reason I know there is a memory leak is because when the application starts the memory use shoots up to more than 2gb before crashing.

DavidColson
  • 8,387
  • 9
  • 37
  • 50
  • 4
    This is what `unique_ptr` is for :) – Sergey Kalinichenko Oct 01 '12 at 17:04
  • How did you determine that you have a memory leak? The code you've shown will delete all of the pointers held in the `gameEntitys` object; after doing that you should `clear()` it, of course. – Pete Becker Oct 01 '12 at 17:08
  • And did you try deleting a single `Entity` and checking for a memory leak? – Beta Oct 01 '12 at 17:09
  • 1
    The question is why he is using dynamic allocation here in the first place. Until we know that, and what else he is doing with the vector, we can't really say what he should be doing. (A priori, in such cases, the solution is `std::vector`. But the name of the class makes me suspect that something else is in play.) – James Kanze Oct 01 '12 at 17:09
  • 2
    And just a guess: he's using a pointer because the objects are polymorphic. And `Entity` doesn't have a virtual destructor. – James Kanze Oct 01 '12 at 17:10
  • What's the point of the word "correctly" in the question? Were you assuming that we would give an *incorrect* solution if you didn't specify that? – Kerrek SB Oct 01 '12 at 17:20
  • 1
    @Kerrek: I assume, "correctly by contrast with what I'm currently doing, which appears incorrect". But you're right that it's redundant. – Steve Jessop Oct 01 '12 at 17:36
  • @SteveJessop: If a question concerns already-correct code, it doesn't belong on StackOverflow, does it :-) – Kerrek SB Oct 01 '12 at 17:36

1 Answers1

7

Most likely, the memory leak is coming from the destructor of your Entity or one of its subclasses: you are performing your delete operations correctly, so the destructor itself must be at fault. One common problem is not making destructors virtual in a polymorphic class hierarchy.

You are also absolutely right about uselessness of calling gameEntitys.clear(): it will leak your objects "wholesale" by "forgetting" the references to them.

If you are on C++11, consider changing the definition to use std::unique_ptr

std::vector<std::unique_ptr<Entity> > gameEntitys;

This will free you from the need to manage the memory of your entries manually, while letting you keep pointers to objects of derived classes in your vector. If you use unique_ptr, calls to gameEntitys.clear() will destroy the items pointed to by vector's elements.

Dealing with the vector of unique_ptr is somewhat different (for example, inserting new items requires extra care, see this answer for details) but I think the positives of simplified memory management compensate for the slight inconveniences.

EDIT : Since your Entity class is not polymorphic, consider switching to std::vector<Entity>, unless you plan to switch to a polymorphic hierarchy later on.

Community
  • 1
  • 1
Sergey Kalinichenko
  • 714,442
  • 84
  • 1,110
  • 1,523
  • This will also change the syntax of accessing the vector. He can't read into an `Entity*` directly, because there's no implicit conversion. And he certainly doesn't want to read into an `std::unique_ptr`, since this will result in a dead pointer in the vector. – James Kanze Oct 01 '12 at 17:12
  • @JamesKanze Thanks for the note, I changed the answer to mention that `std::unique_ptr` is not a drop-in replacement. Thank you very much! – Sergey Kalinichenko Oct 01 '12 at 17:19
  • +1. If you can't use c++11 and std::unique_ptr, you could use boost shared_ptr in the same way, but the with some overhead for the ref counting when the vector is modified. – Johan Lundberg Oct 01 '12 at 17:24
  • the problem is the Entity class is not polymorphic, it has no subclasses and there is nothing to delete in its constructor? – DavidColson Oct 01 '12 at 17:25
  • @JohanLundberg But if `Entity` means what I think it means, the overhead won't be measurable compared to the rest. – James Kanze Oct 01 '12 at 17:25
  • @DavidC If `Entity` isn't polymorphic, why are you storing pointers? Why not just `std::vector`. (And if `Entity` isn't polymorphic, or if it has a virtual destructor, your code is correct. Or rather, it formally has undefined behavior, but in practice, it's inconceivable that there be an implementation where it won't work. (It's one of the rare cases of undefined behavior that even I don't worry about.) – James Kanze Oct 01 '12 at 17:29
  • @DavidC If `Entity` is not polymorphic, do you need pointers because it is really huge?.. – Sergey Kalinichenko Oct 01 '12 at 17:29
  • @DavidC Something called `Entity` sounds to me like it has a lot of behavior. (It also sounds like something that is an abstract base class.) If it has a lot of behavior, the very small extra time necessary to copy a `shared_ptr` will be negligible. – James Kanze Oct 01 '12 at 17:35
  • @JamesKanze, well I'd just go ahead and use shared_ptr (or unique_ptr if he can use c++11) but I can't guess the details of his case. – Johan Lundberg Oct 01 '12 at 18:34