Is this a good way to delete a map of longs and objects made with new
// iterate over the map for (std::map<unsigned long, Object*>::iterator it = objects.begin(), it_end = objects.end(); it != it_end; ++it) { Object* temp = it->second; if(temp) delete temp; } // clear the map objects.clear(); Yes. Use boost::ptr_map
boost::ptr_map<std::string, BigObject> data; data.insert("Plop", new BigObject); When data goes out of scope it deletes all its value members.
Also for algorithms all members are returned as reference to the object (not pointer) thus it is much easier to use with the standard algorithms than a std::map<std::string, BigObject*> where you would need to de-reference the memebrs before use.
One would have to question why you have a map of pointer to int/long in the first place? Would it not be easier to just to store the value in the map?
Object classes. I presume it's done for polymorphism.
std::unique_ptr works on VC2010 and g++ 4.6, and if you have g++ 4.5 you can use std::shared_ptr (although std::unique_ptr is supported by g++ 4.5, std::map is not yet move-aware so you can't use it with that; this was fixed in 4.6).Yes, although the better solution would be to use smart pointers instead of Object*, but that is a different subject.
You could shorten the content of the for to
{ delete it->second; } delete null is defined and is a noop.
It is a perfectly fine way to delete.
Though you can make your life much easier by using a smart pointer.
It's a good start.
Once you've deleted the object, you should either remove the pointer from the map or set it to NULL to avoid dangling pointers. Edit: or of course you could just clear out the map when you're done, as your example shows.
When you store pointers in any of the standard containers, there's always a possibility that an exception or some code error will result in a memory leak. I'd suggest boost pointer containers as an alternative.
Yes. Regardless of what might be considered a design or architecture issue (the use of smarter pointers, for example), it is not unreasonable to use that method to clear the map.
One proviso: the objects map should not by changed by the destruction of the Object* pointers. If it is, it might be better to use something like this:
// iterate over the map typedef std::map<unsigned long, Object*>::iterator map_iter; for (map_iter it = objects.begin(); it != objects.end(); /* blank */ ) { iter_map to_delete = it; ++it; Object* temp = to_delete->second; if(temp) delete temp; objects.delete(to_delete); } I would recommend the use of a smart pointer, for example std::unique_ptr (this is a C++0x feature, not all compilers support it yet). For example:
std::map<unsigned long, std::unique_ptr<Object>> map; // Do something with the map map.clear(); // The objects are automatically deleted. You can also use std::shared_ptr (or boost::shared_ptr if your compiler doesn't support C++0x smart pointers), which has the advantage that it will work if your map can contain the same pointer more than once, and your objects won't be destroyed if someone else still has a pointer to them.
boost::ptr_map is also an options, though I believe that, like the manual approach, will not work right if the map contains the same pointer more than once.
