Memory Leaks - STL sets

问题:

I am trying to plug up all my memory leaks (which is massive). I am new to STL. I have a class library where I have 3 sets. I am also creating a lot of memory with new in the library class for adding info to the sets...

Do I need to deallocate the sets? If so, how?

Here is the library.h

    #pragma once

#include <ostream>
#include <map>
#include <set>
#include <string>
#include "Item.h"

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* const item, const string& keyword);
    const ItemSet* itemsForKeyword(const string& keyword) const;
    void printItem(ostream& out, const Item* const item) const;

    // book-related functions

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

    // music-related functions

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

    // movie-related functions

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

I am not sure what i need to do for the destructor?

Library::~Library()
{


}

also, am I de allocating the stringset right?

    #ifndef CD_H
#define CD_H
#pragma once
#include "item.h"
#include <set>


typedef set<string> StringSet;


class CD : public Item
{
public:

    CD(const string& theTitle, const string& theBand, const int snumber);
    void addBandMember(const string& member);
    const int getNumber() const;
    const StringSet* getMusician() const;
    const string getBand() const;
    virtual void print(ostream& out) const;
    string printmusicians(const StringSet* musicians) const;

    ~CD();


private:

    string band;
    StringSet* music;

    string title;
    int number;

};

ostream& operator<<(ostream& out, const CD* cd);

#endif

cd.cpp

    #include "CD.h"

using namespace std;

CD::CD(const string& theTitle, const string& theBand, const int snumber)
: Item(theTitle), band(theBand),number(snumber), music(new StringSet)
{



}

CD::~CD()
{

    delete []music;

}

in the library class I am creating a lot of memory, but dont the destructor clean that up? example:

    const Item* Library::addBook(const string& title, const string& author, const int nPages)
{

    ItemSet* obj = new ItemSet();
    Book* item = new Book(title,author,nPages);
    allBooks.insert(item); // add to set of all books
    obj->insert(item);

Note: I do not have a copy constructor. I am not sure if I even need one or how top add one. I dont think my destructors are getting called either..

回答1:

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();

回答2:

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).

回答3:

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.

回答4:

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!

回答5:

Looking at some of the code you posted in other questions (such as https://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.

回答6:

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;
}

回答7:

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.