CODEWITHSUNDEEP
c++pointersdictionary
NL3
NL3

Reputation: 127

C++ map with pointers that are automatically freed upon exit, how to handle delete

I have a map that uses strings as keys and pointers of a custom object as value. During runtime, the user can create instances of this object (a pointer) that is automatically added to the map. I use DrMemory to see if I have any leaks (e.g. memory leaks).

So I tested the application without manually deleting the pointers contained the map and no errors (hence, also no memory leaks) were found. To be on the safe side, I also did a test with a method that is called upon exiting the program which iterates through the map and deletes each pointer manually.

When I now run a test on it, I get several errors of the same kind:

UNADDRESSABLE ACCESS of freed memory ...

The line of occurrence is pointing to the location of the clean up method.

My question now is: Should I keep the call to the clean up method (and hence just ignore the errors) or should I remove the call and leave it as it is knowning that it is not necessary to manually delete the pointers?

I am deleting the pointer and erase the entry from the map

void cleanUp(std::map <string, Car*>* map){
    for(auto const& x : *map){
       delete x.second;
        map->erase(x.first);
    }
}

Upvotes: 2

Views: 1940

Answers (3)

marcinj
marcinj

Reputation: 49986

Its hard to tell without seeing sourcode, but keeping bare pointers to dynamically allocated objects (pointers not protected by RAII implementing class) anywhere is a sign of bad practice. Learn to use smart pointer and you will not need to use any memory leak detectors, it really is possible to code in C++ without worring about memory leaks, example code which uses unique_ptr:

std::map<std::string, std::unique_ptr<std::string>> mm;
mm["alpha"] = std::make_unique<std::string>("alpha");

Upvotes: 5

M.M
M.M

Reputation: 141574

Your loop is wrong. The range-based for-loop is equivalent to iterating over the map with an iterator, but when you delete the element you're currently on, the iterator becomes invalid. (The ++ operation to get the next element will then try to use a link that's been freed or whatever).

Instead you could write:

for(auto const& x : *map){
    delete x.second;

map->clear();

This would probably be more efficient as it is fast to blow away the whole map, but slow to remove items one at a time.

NB. Consider using smart pointers in the map, so that items are automatically freed when the map is cleared.

Upvotes: 2

uh oh somebody needs a pupper
uh oh somebody needs a pupper

Reputation: 3396

No, it's not wise to ignore the errors. You are using DrMemory presumably to find problems with your code, so why ignore it? Just because your program runs fine doesn't mean it is fine. Good chances are there's undefined behavior in your code. It may appear to work correctly, subtly break or crash. And you definitely don't want your program to crash once it's been released to your users.

You should erase items from the map that have any invalidated pointers, or better yet, use a smart pointer. However a map containing pointer values seems kind of like an anti-pattern. Consider this:

map[1] = new Foo();
map[1] = new Foo(); // Oops, you're now leaking memory!

How will you delete the previous Foo()?

Upvotes: 0

Related Questions