what's the best way to delete an entry map<int, A*>

I discussed with my coworker about how to delete an entr开发者_运维技巧y in a map the map has the int as the index and a pointer pointing to an object.

I said, first release the object, and then delete the entry. My coworker said first delete the entry and then release the object.

So what's the best way? Any trick for this question?

Delete object first, then remove from map. Otherwise you're just introducing a pointless intermediate variable for storing the pointer. As long as you're singlethreaded, or have proper locking in a multithreaded scenario, the two methods are for all practical purposes equivalent.

map<int, A *>::iterator it = mymap.find(1);
if (it != mymap.end()) {
  delete it->second;
  mymap.erase(it);
} 

Unless you have a multi-threaded environment, either way would work. The rule of thumb is that, once your function returns, there should be no dangling pointers left, i.e. no pointers to the object that just was deleted.

The only problem that could occur is that if you delete the entry first, you have to make sure to have a temporary copy of the pointer, as you will not be able to retrieve it from the map after the entry has been deleted.

Inspired by @Nim, how about a third way: Store the objects in the map by value or by smart pointer. Then RAII will take care of all the cleanup for you automatically!

If you must use raw pointers then it doesn't really matter, as long as you make sure that any required locking for threading concerns is applied.

The conservative approach is to erase first, then delete the pointer.

Storing an invalid pointer value in a Standard Container may lead to undefined behavior, at least according to a common interpretation of the C++ Standard's paragraph [basic.stc.dynamic.deallocation]/4, which prohibits any use of invalid pointer values (such as a container making a copy of the pointer internally), and [lib.container.requirements], which mandates that objects stored in containers must be CopyConstructible and Assignable.

The issue is somewhat contentious, however.

It probably doesn't matter.

You might think that it would matter if you had two threads, one trying to delete an item from the map and another trying to access the same map. You might arrive at the conclusion that it would be safer to remove the item from the map first, so that the other thread doesn't retrieve a pointer to a deleted object.

However, if you have multiple threads accessing the same map, you need to protect it using a synchronisation object (a mutex, or CRITICAL_SECTION if you're in plain Win32.) std::map isn't safe for unsynchronised multi-threaded use where one thread is modifying the collection. So, if you are already locking the map while removing and deleting, it doesn't matter which way around you're doing it.

Having said this, if destruction of your object takes a long time, you might like to move the delete call outside the mutex-locked portion of code. In this case, first store the object's pointer in a temporary variable, remove its entry in the map, unlock, then delete.

You should look into using ptr_map from boost. There is no reason for you to roll up your own solution for this. Do listen to everyone who is pointing out caveats with multithreaded access to your containers.