How to avoid memory leaks when using a vector of pointers to dynamically allocated objects in C++?
Asked Answered
B

4

78

I'm using a vector of pointers to objects. These objects are derived from a base class, and are being dynamically allocated and stored.

For example, I have something like:

vector<Enemy*> Enemies;

and I'll be deriving from the Enemy class and then dynamically allocating memory for the derived class, like this:

enemies.push_back(new Monster());

What are things I need to be aware of to avoid memory leaks and other problems?

Bluepoint answered 1/9, 2009 at 7:58 Comment(5)
Maybe a native English speaker can decipher what you want to say, but I'm lost. First, you're talking about memory leaks -> language/platform dependent; I expect you mean C++. Avoiding memory leaks has been discussed extensively already (stackoverflow.com/search?q=c%2B%2B+raii). You need a virtual destructor for deleting from a base type to be working correctly.Dreibund
What do you mean by "vectors to pointers"? Do you mean "vectors of pointers"?Tuddor
yes, I'm using C++. Yes, I do mean vectors of pointers. Sorry for my bad EnglishBluepoint
I took a shot at rewording it all, please edit or comment if I've removed any information, or if it's not clear.Salina
Only that you need to delete each element of the vector of pointers to new classes defined within the vector. The vector container itself will be deallocated automatically when it goes out of scope. Note if your inheritance hierarchy is virtual, then you need to explicitly define your destructors, as that may also cause memory leaks.Suhail
S
165

std::vector will manage the memory for you, like always, but this memory will be of pointers, not objects.

What this means is that your classes will be lost in memory once your vector goes out of scope. For example:

#include <vector>

struct base
{
    virtual ~base() {}
};

struct derived : base {};

typedef std::vector<base*> container;

void foo()
{
    container c;

    for (unsigned i = 0; i < 100; ++i)
        c.push_back(new derived());

} // leaks here! frees the pointers, doesn't delete them (nor should it)

int main()
{
    foo();
}

What you'd need to do is make sure you delete all the objects before the vector goes out of scope:

#include <algorithm>
#include <vector>

struct base
{
    virtual ~base() {}
};

struct derived : base {};

typedef std::vector<base*> container;

template <typename T>
void delete_pointed_to(T* const ptr)
{
    delete ptr;
}

void foo()
{
    container c;

    for (unsigned i = 0; i < 100; ++i)
        c.push_back(new derived());

    // free memory
    std::for_each(c.begin(), c.end(), delete_pointed_to<base>);
}

int main()
{
    foo();
}

This is difficult to maintain, though, because we have to remember to perform some action. More importantly, if an exception were to occur in-between the allocation of elements and the deallocation loop, the deallocation loop would never run and you're stuck with the memory leak anyway! This is called exception safety and it's a critical reason why deallocation needs to be done automatically.

Better would be if the pointers deleted themselves. Theses are called smart pointers, and the standard library provides std::unique_ptr and std::shared_ptr.

std::unique_ptr represents a unique (unshared, single-owner) pointer to some resource. This should be your default smart pointer, and overall complete replacement of any raw pointer use.

auto myresource = /*std::*/make_unique<derived>(); // won't leak, frees itself

std::make_unique is missing from the C++11 standard by oversight, but you can make one yourself. To directly create a unique_ptr (not recommended over make_unique if you can), do this:

std::unique_ptr<derived> myresource(new derived());

Unique pointers have move semantics only; they cannot be copied:

auto x = myresource; // error, cannot copy
auto y = std::move(myresource); // okay, now myresource is empty

And this is all we need to use it in a container:

#include <memory>
#include <vector>

struct base
{
    virtual ~base() {}
};

struct derived : base {};

typedef std::vector<std::unique_ptr<base>> container;

void foo()
{
    container c;

    for (unsigned i = 0; i < 100; ++i)
        c.push_back(make_unique<derived>());

} // all automatically freed here

int main()
{
    foo();
}

shared_ptr has reference-counting copy semantics; it allows multiple owners sharing the object. It tracks how many shared_ptrs exist for an object, and when the last one ceases to exist (that count goes to zero), it frees the pointer. Copying simply increases the reference count (and moving transfers ownership at a lower, almost free cost). You make them with std::make_shared (or directly as shown above, but because shared_ptr has to internally make allocations, it's generally more efficient and technically more exception-safe to use make_shared).

#include <memory>
#include <vector>

struct base
{
    virtual ~base() {}
};

struct derived : base {};

typedef std::vector<std::shared_ptr<base>> container;

void foo()
{
    container c;

    for (unsigned i = 0; i < 100; ++i)
        c.push_back(std::make_shared<derived>());

} // all automatically freed here

int main()
{
    foo();
}

Remember, you generally want to use std::unique_ptr as a default because it's more lightweight. Additionally, std::shared_ptr can be constructed out of a std::unique_ptr (but not vice versa), so it's okay to start small.

Alternatively, you could use a container created to store pointers to objects, such as a boost::ptr_container:

#include <boost/ptr_container/ptr_vector.hpp>

struct base
{
    virtual ~base() {}
};

struct derived : base {};

// hold pointers, specially
typedef boost::ptr_vector<base> container;

void foo()
{
    container c;

    for (int i = 0; i < 100; ++i)
        c.push_back(new Derived());

} // all automatically freed here

int main()
{
    foo();
}

While boost::ptr_vector<T> had obvious use in C++03, I can't speak of the relevance now because we can use std::vector<std::unique_ptr<T>> with probably little to no comparable overhead, but this claim should be tested.

Regardless, never explicitly free things in your code. Wrap things up to make sure resource management is dealt with automatically. You should have no raw owning pointers in your code.

As a default in a game, I would probably go with std::vector<std::shared_ptr<T>>. We expect sharing anyway, it's fast enough until profiling says otherwise, it's safe, and it's easy to use.

Salina answered 1/9, 2009 at 8:24 Comment(14)
If he's actually writing gamecode (as the example kind of alludes to) then a ref counted pointer (or however boost implemented the shared pointer) is likely overly expensive.. a constant memory footprint (especially for AI objects) is a loftier design goal than removing a for loop to deallocate.Nephridium
Which one should I choose b/w Pointer Contains and Shared Pointers and why?Bluepoint
@Dan: Some way or another you will have to do the cleanup and if that's too slow, the question isn't which way to do it, but how to avoid having to do it in the first place. If you can't get around it, use the cleanest way first, then measure, and only try to improve afterwards. Boost means several thousand pairs of keen eyes improving the code. Hard to beat that: I have seen boost's shared_ptr outperforming a custom smart pointer using a special-purpose allocator in CPU/GPU-intensive 3D applications. Until you measure, you never know...Unloosen
Updated my answer. Luckily our 'answers' matched this time, sbi. :P (Profile!)Salina
GMan, I wasn't aware that our answers matching is something to note. :^> I usually find myself agreeing with you and you're among those who's answers I value (and upvote regularly). Have we disagreed a lot recently?Unloosen
Ha, no I was referring to my comments on your answer below on my unsureness to edit in your information. My comment isn't an issue if your information is my answers. Lame connection. :P Anyway, thank you for the compliment.Salina
@GMan: Now you have me totally confused. :o> I guess I'll just stick to "isn't an issue" then, OK?Unloosen
Thanks sir, really helpful and detailed answerBluepoint
@Unloosen I'm not advocating a different shared_ptr, I'm advocating a different approach to memory management. Shared pointers are very likely inappropriate in the game code case. In fact, they're totally inappropriate for the example the original poster submitted. Most of my argument is summarized here: bureau14.fr/blogea/2009/08/smart-pointers-are-overusedNephridium
@Dan: The article you linked to had (besides sound advice to not to mindlessly copy around smart pointers) a nice example of a dynamically allocated object that lives for the applications lifetime. I suppose that's quite different from the most likely constantly changing number of enemies in a computer game. These things will most likely have to be allocated dynamically and a way has to be found to manage their lifetime. Smart pointers are (IMO) the default off-the-shelf solution to this and I wouldn't use anything else unless profiling forces me to.Unloosen
This answer is referenced by others, but it's a bit dated now. It might be worth revising to mention std::unique_ptr instead of std::auto_ptr or boost::shared_ptr.Oak
i'm getting a segfault when deleting an element of the vector, although I allocate it with new. (I can't use smart pointers)Inquiline
@CoolMikeWhoDoesSomething: I'd recommend asking a new question for your issue. Note that if you don't have any smart pointers readily available, it is highly worth the time to write your own unique_ptr and go from there.Salina
@Salina I don't want to make a question, because my code is too complicated to put in a stackoverflow question and I don't think that the problem will continue to exist if I make the code simple. Although thanks for your advice, because my own smart pointers will be very nice for the project I'm working on :)Inquiline
U
10

The trouble with using vector<T*> is that, whenever the vector goes out of scope unexpectedly (like when an exception is thrown), the vector cleans up after yourself, but this will only free the memory it manages for holding the pointer, not the memory you allocated for what the pointers are referring to. So GMan's delete_pointed_to function is of limited value, as it only works when nothing goes wrong.

What you need to do is to use a smart pointer:

vector< std::tr1::shared_ptr<Enemy> > Enemies;

(If your std lib comes without TR1, use boost::shared_ptr instead.) Except for very rare corner cases (circular references) this simply removes the trouble of object lifetime.

Edit: Note that GMan, in his detailed answer, mentions this, too.

Unloosen answered 1/9, 2009 at 8:41 Comment(5)
@GMan: I completely read your answer and saw this. I would have only mentioned the delete_pointer_to possibility without elaborating on it, since it's so much inferior. I felt the need to put the off-the-shelf solution into a short, simple "do-it-this-way" answer. (Boost's pointer containers are a nice alternative, though, and I did give an upvote for mentioning them.) I'm sorry if you felt misread.Unloosen
I think your point is very good, actually. Should I edit it in? I'm always unsure at this point. If I edit my answer so it's more complete, I feel like I'm "stealing" rep from other people.Salina
@GMan: Go ahead and improve the answer that's on top of the stack. Your answer is good and detailed and definitley deserves to be there. To hell with the rep, if there's one less programmer out there doing this kind of things, that will help us all a lot more than any rep points. :)Unloosen
and perhaps will help others in future, thus saving others time :)Bluepoint
My word! Friendly and cooperative discourse, let alone agreement in an online discussion? Totally unheard of! Nice work :)Lankford
S
9

I am assuming following:

  1. You are having a vector like vector< base* >
  2. You are pushing the pointers to this vector after allocating the objects on heap
  3. You want to do a push_back of derived* pointer into this vector.

Following things come to my mind:

  1. Vector will not release the memory of the object pointed to by the pointer. You have to delete it itself.
  2. Nothing specific to vector, but the base class destructor should be virtual.
  3. vector< base* > and vector< derived* > are two totally different types.
Summon answered 1/9, 2009 at 8:4 Comment(2)
Your assumptions are absolutely correct. Sorry, I wasn't able to explain properly. Is there anything else?Bluepoint
If possible avoid raw pointers, and use the methods described in GMan's answer.Summon
S
0

This line suggests that the vector will be owning the pointers:

 enemies.push_back(new Monster());

A good practice is that we should never use a raw pointer to own an object, so we should instead have:

std::vector<std::unique_ptr<Enemy>> Enemies;

and

    enemies.push_back(std::make_unique<Monster>());

We can pass references or pointers (from unique_ptr.get()) into functions or other objects that don't need to own an Enemy object (because their duration is definitely less than that of the enemy itself).

Destructor of std::vector invokes its contents' destructors, so lifetime is guaranteed to terminate when the vector is destroyed (or when its elements are erased).

If ownership (and control of lifetime) needs to be shared, then we'll need to use std::shared_ptr<Enemy> in all the objects that share ownership (still use references or raw pointers in non-owning contexts, of course). That's harder to reason about (and uses more resources), so std::unique_ptr should be your preference where possible.

Saad answered 2/2 at 10:31 Comment(0)

© 2022 - 2024 — McMap. All rights reserved.