What is the most concise way to clear a map and delete contents?

Question

I have a std::list and a std::map that I would like to empty and call delete on all the pointers.

After reading this question I am using this for the std::list:

mylist.remove_if([](myThingy* thingy) -> bool { delete thingy; return true; });

Is there something similarly concise for std::map?

Note that I cannot use a range based for loop because it isn't supported by my compiler (VC10). If it would be possible I assume

for(auto* thingy : myMap) { delete thingy; }
myMap.clear();

would work. Please correct me if I am wrong.

You really shouldn't use `std::list` or `std::map` for collections of ordinary pointers. You should either use collections designed to hold pointers or use smart pointers. If you do either of those, you can do what you want just by emptying the collection. — David Schwartz, May 03 '13 at 17:14
It's old code by somebody else, but still, which collections are designed to use pointers? — Sarien, May 03 '13 at 17:18
I second Sarien's question: "Which collections are designed to use pointers?" — freitass, May 03 '13 at 17:29
boost::ptr_container http://www.boost.org/doc/libs/1_53_0/libs/ptr_container/doc/reference.html — Stephan Dollberg, May 03 '13 at 17:31
@DavidSchwartz `std::map` for raw pointers is ubitquitous for navigation purposes. (Here, of course, it is clear that the `map` should manage the pointers, and `std::shared_ptr would probably be a good idea.) — James Kanze, May 03 '13 at 17:40
@JamesKanze: I vote for `unique_ptr` if the `map` is the owner. — Jon Purdy, May 03 '13 at 17:53
@JonPurdy I vote for _never_ putting a `unique_ptr` into a container. Someone, sometime later, will end up writing something like `auto ptr = myMap[key];`, for example. _If_ you need containers to manage pointers (and in my experience, the need is rare), then the best solution is probably one of Boost's pointer containers. Otherwise, `std::shared_ptr`, which actually works very well with the `auto` above. — James Kanze, May 04 '13 at 11:27
@JamesKanze: `auto ptr = myMap[key];` results in “call to implicitly deleted copy constructor of `std::unique_ptr<...>`”. If you want to transfer ownership, you must explicitly `move`. Boost’s pointer containers do precisely the same thing as `unique_ptr`, except with the ownership policy coupled to the container. — Jon Purdy, May 04 '13 at 18:20

score 2 · Accepted Answer · edited Jun 20 '20 at 09:12

Is there something similarly concise for std::map?

You could do it this way (supposing your thingy is the mapped value, and not the key):

for_each(myMap.begin(), myMap.end(), 
    [] (decltype(myMap)::value_type const& p) { delete p.second; });

myMap.clear();

Concerning the range-based for loop:

If it would be possible I assume
for(auto* thingy : myMap) { delete thingy; }
myMap.clear();
would work. Please correct me if I am wrong.

More or less. You still need to keep in mind that values of a map are actually pairs - but you could fix the range-based for to keep that into account, yes:

for (auto&& p : myMap) { delete p.second; }
myMap.clear();

Anyway, please consider using smart pointers instead of performing manual memory management through raw pointers, new, and delete. This way you would avoid this kind of problems.

I had to change the lambda to [](std::pair p) { delete p.second; }. Somehow VC10 didn't like the decltype. — Sarien, May 03 '13 at 17:45
@Sarien: Oh, ok then. VC10 weirdness. You could probably have a `typedef std::map my_map;`, or `typedef decltype(myMap) my_map;` and define the lambda as `[] (my_map::value_type const& p) { ... }`, it should accept that — Andy Prowl, May 03 '13 at 17:46

score 2 · Answer 2 · answered May 03 '13 at 17:38

I don't think your remove_if solution is a good idea for the other types, either. Just use a simple loop (or foreach):

for ( auto current = myMap.begin(); current != myMap.end(); ++ current ) {
    delete current->second;
}
myMap.clear();

Note that you cannot do a delete current->first; this will invalidate keys in the map. And unless you are doing a clear() immediately afterwards (or are destructing the map), set the deleted pointer to NULL.

With regards to your second question:

for ( auto* thingy : myMap )

would certainly not work. The value type of map is a pair, not a pointer, so you'ld need somthing like:

for ( auto thingy : myMap ) { delete thingy.second; }

(I think. I've not been able to experiment with this either.)

@Sarien It's not that I think it might not be a good idea. I'm just not sure of the exact syntax. It's one thing reading the standard, or some book, and another trying to use it. — James Kanze, May 04 '13 at 11:23

What is the most concise way to clear a map and delete contents?

2 Answers2