Correctly delete pointers in std::list allocated elsewhere

Question

Possible Duplicate:
Does std::list::remove method call destructor of each removed element?

I have a SpriteHandler class that allows the user to register a pointer to a Sprite object for drawing, all it does is access methods on the object. I wanted to write a safety catch that automatically deleted the memory associated with the pointers if the user forgot to do so by the end of the program (and it's less to worry about for the user too!) :

 

//SpriteHandler.h
class SpriteHandler {
public:
//...
    void RegisterObject(Sprite* object);
    bool IsRegistered(Sprite* object);
    void UnregisterObject(Sprite* object);
private:
//...
    static std::list<Sprite*>* _sprite = NULL;
};

//SpriteHandler.cpp
std::list<Sprite*>* SpriteHandler::_sprites = NULL;


void SpriteHandler::RegisterObject(Sprite* object) {
    if(object == NULL) return;
    if(_sprites == NULL) _sprites = new std::list<Sprite*>();
    _sprites->push_back(object);
    _sprites->sort(UDLessSprite);
}

bool SpriteHandler::IsRegistered(Sprite* object) {
    return std::binary_search(_sprites->begin(), _sprites->end(), object);
}

void SpriteHandler::UnregisterObject(Sprite* object) {
    if(object == NULL) return;
    if(IsRegistered(object) == false) return;

    _sprites->remove(object);
    if(_sprites->size() <= 0) {
        if(_sprites) {
            _sprites->clear();
            delete _sprites;
            _sprites = NULL;
        }
        return;
    }
    _sprites->sort(UDLessSprite);
}

void SpriteHandler::Release() {
    if(_sprites) {
        std::list<Sprite*>::iterator _iter = _sprites->begin();
        while(_iter != _sprites->end()) {
            delete (*_iter);
            (*_iter) = NULL;
            ++_iter;
        }
        _sprites->clear();
        delete _sprites;
        _sprites = NULL;
    }
}

 

The issue I"m having is that after the first pointer is deleted the next iterator is pointing to an already freed object (memory location is 0xfeeefeee).

How would I correctly iterate through them, deleting each one?

Answer 1

If you want safety and implicit resource cleanup, do not use raw pointers, use smart pointers!

The problem with STL containers is:
If the contained object is a pointer STL containers DO NOT take ownership of destroying it. You will have to explicitly call delete on each contained pointer to delete the content it is pointing to.

Have a look at this similar question here.

The best way to go about this is not storing raw pointers inside STL containers but using their intelligent cousins smart pointers instead(boost::shared_ptr) Check out the Boost documentation, These pointer cousins are intelligent enough to deallocate themselves when there is no one referring to them and saves you the problems like the one you are facing now.

Answer 2

There are a lot of things wrong with this code. But they all stem from this line:

std::list<Sprite*>* _sprite = NULL;

Unless you're using C++0x, this doesn't compile. You can't set the value of non-static members like this. Unless you intended for this to be static, and if you did, you should have used the static keyword.

But even worse is the fact that you're allocating a std::list on the heap. Why? You allocate one when you need it, and deallocate it in the destructor. Just make it a regular member variable.

C++ is not Java. Not everything has to be pointers.

If you are claiming ownership of these Sprite objects, you need to actually claim ownership of them. This would preferably be done with some kind of smart pointers. At a minimum, you should have a std::list<std::auto_ptr<Sprite> > This will ensure that the Sprite objects are deleted when you remove the entries from the list. If you have access to Boost (and if you don't, you need to get access to it) you can use boost::shared_ptr. C++0x offers much the same with std::shared_ptr.

std::auto_ptr only allows one object to own the pointer, while boost::shared_ptr allows shared ownership (hence the name). That's not necessary for your code (from what we can see at least), but it would not be a bad idea to allow shared ownership of Sprite objects. In C++0x, you should use std::unique_ptr instead of std::auto_ptr.

Either way, your code would now look like this:

//SpriteHandler.h
class SpriteHandler {
public:
//...
    void RegisterObject(Sprite* object);
    bool IsRegistered(Sprite* object);
    void UnregisterObject(Sprite* object);
private:
//...
    std::list<boost::shared_ptr<Sprite> > _sprite;
};


void SpriteHandler::RegisterObject(Sprite* object) {
    if(!object) return;
    _sprites.push_back(object);
    _sprites.sort(UDLessSprite);
}

bool SpriteHandler::IsRegistered(Sprite* object) {
    return std::binary_search(_sprites.begin(), _sprites.end(), object);
}

struct SpriteTester{
    SpriteTester(Sprite *testValue) : _testValue(testValue) {}
    bool operator()(const boost::shared_ptr<Sprite> &other) const{
        return other.get() == _testValue;
    }

    Sprite *_testValue;
};

void SpriteHandler::UnregisterObject(Sprite* object) {
    if(object == NULL) return;

    _sprites.remove_if(object, SpriteTester(object));
    //Deleting an entry cannot make the list unsorted.
}

void SpriteHandler::Release() {
    _sprites.clear();
}

Notice that I introduced the SpriteTexture. This is because you cannot pass a Sprite* to std::list::remove now that we're using smart pointers. If you do, it will wrap it in a boost::shared_ptr temporary, thus causing the pointer to be deleted. This is bad, so I had to use a custom tester.

Also, if you want to have Sprite objects registered with a class, then the Sprite object constructor (or a factory method) should be doing this registering. The user should not be able to create unregistered Sprites.