ansaurus

Question

Answer 1

+1 A:

haven't gone through all of your code but from the first few lines it seems you're maintaining sets of pointers. Whenever you have an STL container which holds pointers and you're using new to put stuff in the pointers, you must use delete to deallocate these pointers. STL doesn't do that for you. In fact, STL doesn't even know they are pointers.

Another option is to not use pointers at all and have a set of just the objects and not use new to create them. Just create them on the stack and copy them into the set.

shoosh 2010-03-06 23:48:33

Answer 2

A:

In the destructor you need to iterate over your stl collections that contain pointers and delete them. Like this:

while (!collection.empty()) {
    collection_type::iterator it = collection.begin();
    your_class* p = *it;
    collection.erase(it);
    delete p;
}

pajton 2010-03-06 23:51:45

Answer 3

+2 A:

You need to free the memory for each element of the set. The container will not do that for you, and it shouldn't because it can't know whether it owns that data or not -- it could just be holding pointers to objects owned by something else.

This is a generic free function that will deallocate any STL container.

template <typename T>
void deallocate_container(T& c)
{
  for (typename T::iterator i = c.begin(); i != c.end(); ++i)
    delete *i;
}

// Usage
set<SomeType*> my_set;
deallocate_container(my_set);
my_set.clear();

Peter Alexander 2010-03-06 23:58:53

But you need RTTI for that, hah!

pajton 2010-03-07 00:06:44

Why would you need RTTI for this?

Peter Alexander 2010-03-07 00:23:47

It probably needs the classes to have virtual destructors. Most classes need virtual destructors anyway. The problem is a property of the class, not this deallocate_container function, IOW. One issue though - does it work for std::map, which has key and data for each item, either or both potentially a pointer?

Steve314 2010-03-07 00:37:38

Yeah, it requires a virtual destructor, but that's orthogonal to the use of this function. And yeah, it won't work with maps because the iterators point to pairs. For maps, you could have dynamically allocated memory in the key and/or value, so you'd need to write versions for those specially.

Peter Alexander 2010-03-07 00:43:16

Answer 4

A:

Well, this might be a stupid comment, but do you really need to have ALL your stuff heap-allocated (aka. using pointers and new ?)

Can't you just use plain instances ? RAII allows easier code and no memory leak.

For example have :

using namespace std;

typedef set<Item>              ItemSet;
typedef map<string,Item>       ItemMap;
typedef map<string,ItemSet>    ItemSetMap;

class Library
{

public:
    // general functions

    void addKeywordForItem(const Item & item, const string& keyword);
    ItemSet itemsForKeyword(const string& keyword) const;
    void printItem(ostream& out, const Item & item) const;

    // book-related functions

    Item addBook(const string& title, const string& author, int nPages);
    ItemSet booksByAuthor(const string& author) const;
    ItemSet books() const;

    // music-related functions

    Item addMusicCD(const string& title, const string& band, int nSongs);
    void addBandMember(const Item & musicCD, const string& member);
    ItemSet musicByBand(const string& band) const;
    ItemSet musicByMusician(const string& musician) const;
    ItemSet musicCDs() const;

    // movie-related functions

    Item addMovieDVD(const string& title, const string& director, int nScenes);
    void addCastMember(const Item & movie, const string& member);
    ItemSet moviesByDirector(const string& director) const;
    ItemSet moviesByActor(const string& actor) const;
    ItemSet movies() const;
    ~Library();
};

With this approach, the destructor has to do strictly nothing, and no memory leak. In most cases using pointers can be easily avoided, and definetly should!

Tristram Gräbener 2010-03-06 23:59:13

Answer 5

A:

As others have noted, you need to deallocate the pointers. The set destructor normally wouldn't do this for you. Otherwise, if you want this to be done for you, use a boost::scoped_ptr or a std::tr1::shared_ptr where you can specify a custom deleter to do this job for you.

dirkgently 2010-03-07 00:00:08

Answer 6

+3 A:

STL containers are not designed to hold pointers.

Look at the boost pointer containers. These containers are designed to hold pointers.

#include <boost/ptr_container/ptr_set.hpp>
#include <boost/ptr_container/ptr_map.hpp>

http://www.boost.org/doc/libs/1_42_0/libs/ptr_container/doc/ptr_set.html

The containers hold and own the pointers so they will be deleted when the container goes out of scope. But the beautiful thing about the containers is that you access the objects via references so all the standard algorithms work without any special adapters.

typedef boost::ptr_set<Item>              ItemSet;
typedef boost::ptr_map<string,Item>       ItemMap;
typedef boost::ptr_map<string,ItemSet>    ItemSetMap;

PS. Its hard to tell accurately but it looks like too many of your interfaces return pointers. It is very rare in C++ to actually return pointers (or pass pointers around). Your interfaces should usually take objects/references or smart pointers (usually in that order but it depends on the situation).

Working with a pointer should be your last resort as there is no clear indication of the owner of the object and thus cleanup becomes the issue (thus leading to massive memory leaks).

Martin York 2010-03-07 00:55:40

+1 especialy about pointers being the last resort

Tristram Gräbener 2010-03-07 18:40:02

Answer 7

A:

Looking at some of the code you posted in other questions (such as http://stackoverflow.com/questions/2376099/c-add-to-stl-set), your items are stored in several global ItemSet objects. This isn't a good design - they really should be part of the Library object, since they logically belong to that.

The best way to fix the memory leaks is not to deal with raw pointers - either store smart pointers in the sets, or use Boost pointer containers as Martin York suggests. Also, your ItemSetMap objects should contain Set objects rather than pointers - there is absolutely no reason to store pointers in them.

If you really must store pointers, then your destructor must walk through each set to delete the contents:

void Purge(ItemSet &set)
{
    for (ItemSet::iterator it = set.begin(); it != set.end(); ++it)
        delete *it;
    set.clear(); // since we won't actually be destroying the container
}

void Purge(ItemSetMap &map)
{
    for (ItemSetMap::iterator it = map.begin(); it != map.end(); ++it)
        delete it->second;
    map.clear();
}

Library::~Library()
{
    Purge(allBooks);
    Purge(allCDs);
    // and so on and so forth
}

but this really isn't how you should be doing it, as just about everyone answering your questions has pointed out.

As for the StringSet, you created it with plain new not new[], so you must delete it with plain delete not delete[]. Or, better still, make music a StringSet object rather than a pointer, then you won't need the destructor at all. Once again, memory management through raw pointers and manual use of delete is error prone, and should be avoided if at all possible.

Mike Seymour 2010-03-07 01:48:33

I still have alot of memory leaks..

icelated 2010-03-07 02:35:59

I also have typedef set<string> StringSet;StringSet* Moviecast; can i just use delete Moviecast on it? or do i have to iterate?

icelated 2010-03-07 02:43:29

No, you don't have to iterate a `set<string>`, since that contains objects and will deallocate them itself. You only need to delete objects that you allocated using `new`, and you should avoid doing that whenever you can.

Mike Seymour 2010-03-07 16:15:24

i used new to add info to my sets in almost all my functions like: ItemSet* obj = new ItemSet(); how do i delete those? i cannot delete those before it leaves the function. - i hope oyu dont mind but i used your purge functions.

icelated 2010-03-07 18:51:25

ansaurus

tags:

views:

answers:

Memory Leaks - STL sets

related questions