C++ STL map issue

[prasadi]

Which is the safe way to delete a newed pointer on map??

int main()
{
map<int,A*> aMap;


A* o1= new A();
A* o2= new A();
A* o3= new A();
A* o4= new A();
A* o5= new A();

aMap.insert(make_pair(1,o1));
aMap.insert(make_pair(2,o2));
aMap.insert(make_pair(3,o3));
aMap.insert(make_pair(4,o4));
aMap.insert(make_pair(5,o5));

aMap.erase(1);
delete o1;
o1 = NULL; // is this safe way?? would this cause any problem in multithreaded applications??

// or follwoing one is the safe way??

delete o1;
o1 = NULL;
aMap.erase(1);


cout<<"Delete success";

return 0;

}

Thanks in advance

[kbw]

At some convenient time, traverse the map (with an iterator), delete the second value, and almost certainly you may want to clear the map.

[prasadi]

both ways can be used to delete a pointer on map, but which is the safe way??

[kbw]

It's not one or the other, it's both.

[prasadi]

you mean both r safe???

[kbw]

No. The first (deleting entries) frees the entires back to the heap, the second (clearing the map) releases dangling references to the heap. Again, you need to do both.

[prasadi]

approach #1

aMap.erase(1);
delete o1;
o1 = NULL; // is this safe way?? would this cause any problem in multithreaded applications??

approach #2

delete o1;
o1 = NULL;
aMap.erase(1);


now tel me which is safe??

[binaryboy]

looks both of ways are safe because when you insert a pair to a map, map will make a copy (or copy construct if any elment of pair is a C++ object ) of pair.

But, you MUST replace

delete o1;
o1 = NULL;
aMap.erase(1);

with

if (o1)
{ 
  delete o1;
  o1 = NULL;
}
aMap.erase(1);

because, your new operation may fail.


and you must make the whole operation atomic such as puts mutex around it if more than one thread can run this operation.

[kbw]

I don't think so. Are you sure about that?

[PanGalactic]

binaryboy is right. If this is a multi-threaded app, you must put a mutex around just about anything involving STL containers. Thread-safety in C++ is very implementation specific.

A more important point is that pointers are rather unsafe (hard to manage) in a threaded environment. Use copies or use a thread-safe shared pointer. Here's why: in a threaded environment, you have to make sure that no object is deleted while any thread has a pointer to that object. You need to protect each object with a lock to ensure the object the pointer points to is not deleted from under you. It doesn't matter which option you choose if another thread is using the pointer when the delete is called. Often a copy is cheaper than managing locks.

[kbw]

There was no mention of an MT environment, that's a different issue.

You don't neet to check for NULL on delete/free.

[prasadi]

Mutex locks are in place already since our application runs in solaris multithreaded enironment.
NULL checks are also taken care, but if use the approach #1

i.e.,

approach #1

aMap.erase(1);
delete o1;
o1 = NULL;

its crashing at "delete o1" ... see the stack trace for the same

ffffffff6f2d40a4 _lwp_kill (6, 0, ffffffff6f3f7338, ffffffffffffffff, ffffffff6f3ec000, 0) + 8
ffffffff6f24a68c abort (1, 1b8, ffffffff6f50786c, 1a1a80, 0, 0) + 118
ffffffff7d900d54 free (25bb18f20, 25bb2a720, 8, ffffffff733e30dc, 1, 25bb18f91) + 1f4
ffffffff6f50786c __1c2k6Fpv_v_ (25bb2a720, 25bb2a720, 25bb2a720, 25bb2a720, 40, 0) + 4
ffffffff733e30dc __1cFpdfcoMCCoSession2T6M_v_ (2e020, 259171ce0, 8, 40, ffffffff73e60bb8, 259171e50) + e74
ffffffff733f8a80 __SLIP.DELETER__P (259171ce0, 1, ffffffff73e60cc7, ffffffff733f8a78, ffffffff73e60cc0, ffffffff733f8a78) +

I'm not seeing any issues with the approach #2
i.e.,

delete o1;
o1 = NULL;
aMap.erase(1);

[binaryboy]

Sorry, you don't neek check for NULL on delete. kbw is right,

but, on my machine:

approach #1

aMap.erase(1);
delete o1;
o1 = NULL; 

it's safe.

My environment is Windows XP-x64 sp2 + Viual Studio 2005 SP1 + Intel PD 920.

so, I think STL implementation maybe different on different platforms...

[prasadi]

not sure , but mine is Solaris 10 and UltraSPARC(T2000 and higher versions)

[kbw]

erase takes an iterator. The code is:

for (p = mymap.begin(); p != mymap.end(); ++p)
    delete p->second;
mymap.clear();

But you shouldn't be able to pass an integer to erase.