Memory Leaks - STL Kits

Question

I am trying to connect all memory leaks (which is massive). I am new to STL. I have a class library where I have 3 sets. I also create a lot of memory with new libraries in the class to add information to collections ...

Do I need to empty kits? If so, how?

Here is the .h library

#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'm not sure what I need to do for the destructor?

 Library::~Library() { } 

Also, am I highlighting stringset correctly?

  #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 create a lot of memory, but does the destructor destroy? 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'm not sure if I even need one or how to add it. I don't think my destructors are called either.

Answer 1

You need to free memory for each item in the set. A container will not do this for you, and it should not, because it cannot know whether it owns this data or not - it can simply contain pointers to objects belonging to something else.

This is a generic free feature that will free up 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(); 

Answer 2

STL containers are not intended to hold pointers.

Look at the containers of the pointer pointer. 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

Containers store and own pointers, so they will be deleted when the container goes out of scope. But the great thing about containers is that you access objects through links, so all 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 say for sure, but it looks like there are too many backward pointer interfaces. In C ++, it is very rare to actually return pointers (or skip pointers). Usually your interfaces should accept objects / links or smart pointers (usually in this order, but it depends on the situation).

Working with the pointer should be your last resort, as there is no clear indication of the owner of the object, and thus cleaning up becomes a problem (which leads to massive memory leaks).

Answer 3

your entire code didn’t go through, but from the first few lines it seems that you support a lot of pointers. Whenever you have an STL container that contains pointers, and you use new to put things in pointers, you should use delete to free these pointers. STL does not do this for you. Actually STL does not even know that they are pointers.

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

Answer 4

Well, that might be a dumb comment, but you really need ALL your stuff to be highlighted in heaps (like using pointers and new ones?)

Can't you use simple instances? RAII allows you to simplify the code and not lose memory.

For example:

 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 should not do anything, and there is no memory leak. In most cases, using pointers can be easily avoided and certainly should be!

Answer 5

By looking at the code that you sent in other questions (for example, https://stackoverflow.com/questions/2376099/c-add-to-stl-set ), your objects are stored in several global ItemSet objects. This is not a very good design - they really should be part of the Library object, since they logically belong to it.

The best way to fix memory leaks is to not deal with raw pointers - either store smart pointers in sets, or use Boost pointer containers, as Martin York suggests. In addition, your ItemSetMap objects must contain Set objects, not pointers, and there is no reason to store pointers in them.

If you really need to store pointers, then your destructor must go through each set to remove 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 that’s really not how you should do it, since almost everything your questions answered indicated.

As for the StringSet , you created it with a simple new not new[] , so you must delete it with the usual delete not delete[] . Or, even better, make music a StringSet object, not a pointer, then you don't need a destructor at all. Once again, managing memory with raw pointers and manually using delete is error prone and should be avoided, if at all possible.

Answer 6

In the destructor, you need to iterate over your stl collections containing pointers and deleting them. Like this:

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

Answer 7

As others have noted, you need to free pointers. The set destructor will usually not do this for you. Otherwise, if you want this to be done for you, use boost::scoped_ptr or std::tr1::shared_ptr , where you can specify the user remote to perform this task.