views:

1460

answers:

3

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?

+5  A: 

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.
Naveen
Your assumptions are absolutely correct. Sorry, I wasn't able to explain properly. Is there anything else?
hab
If possible avoid raw pointers, and use the methods described in GMan's answer.
Naveen
+21  A: 

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(void)
{
    container c;

    for (int i = 0; i < 100; ++i)
        c.push_back(new Derived);
} // leaks here!

int main(void)
{
    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 *p)
{
    delete p;
}

void foo(void)
{
    container c;

    for (int 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(void)
{
    foo();
}

This is unruly, though. Also, 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.

Better would be if the pointers deleted themselves. The standard library provides std::auto_ptr, but this won't do, due to copy issues. You'd need an improved smart pointer, like boost::shared_ptr:

#include <boost/shared_ptr.hpp>
#include <vector>

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

struct Derived : Base
{        
};

// hold smart pointers
typedef boost::shared_ptr<Base> BasePtr;
typedef std::vector<BasePtr> container;

void foo(void)
{
    container c;

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

    // free memory
    // done automatically
}

int main(void)
{
    foo();
}

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>
#include <vector>

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

struct Derived : Base
{        
};

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

void foo(void)
{
    container c;

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

    // free memory
    // done automatically
}

int main(void)
{
    foo();
}

On this basic abstract, ptr_vector and std::vector<boost::shared_ptr> look quite similar; indeed, they will behave the same.

Because performance "matters" in a game, a ptr_container might be slightly better. A shared_ptr carries a bit of extra memory with it. The reason I put matters into quotes is because you probably won't notice the different between the two. You can't optimize something until you know it's a problem, so I'm actually making an assumption that a ptr_vector would be better: I don't know for sure, since we haven't tried it and profiled it.

Personally, though, I would go with a vector of smart pointers. The reason is that once you start using the vector, you could do things such as store the pointer, but remove it from the vector. It's also more RAII- and abstraction-like: The vector holds objects, unknowing what they are or how they work, and the pointers clean up after themselves.

With a ptr_vector, the act of removing a pointer from the vector means what it was pointing to is also deleted, so you can't do something like this safely:

Enemy *e = c.back();
c.pop_back(); // the enemy pointed to by e is deleted, e is now invalid

However, you can do this with a shared_ptr:

typedef boost::shared_ptr<Enemy> EnemyPtr;

EnemyPtr e = c.back();
c.pop_back(); // the enemy is still valid because e holds a reference to it

Because it is reference counted. When you remove it from the vector, the reference count goes down by 1, but will still be at least 1, but you are holding a reference. Once that smart pointer goes out, it will be deleted.

GMan
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.
Dan O
Which one should I choose b/w Pointer Contains and Shared Pointers and why?
hab
@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...
sbi
Updated my answer. Luckily our 'answers' matched this time, sbi. :P (Profile!)
GMan
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?
sbi
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.
GMan
@GMan: Now you have me totally confused. `:o>` I guess I'll just stick to "isn't an issue" then, OK?
sbi
Thanks sir, really helpful and detailed answer
hab
@sbi 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: http://www.bureau14.fr/blogea/2009/08/smart-pointers-are-overused/
Dan O
@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.
sbi
+4  A: 

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.

sbi
I did cover smart pointers directly after that section... :P
GMan
@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.
sbi
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.
GMan
@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. `:)`
sbi
and perhaps will help others in future, thus saving others time :)
hab
Oklie, I've updated :)
GMan
Great!
sbi
My word! Friendly and cooperative discourse, let alone *agreement* in an online discussion? Totally unheard of! Nice work :)
e.James