the correct way to delete a map

[Nick Keighley]

Hi,

I have a map containing pointers. When I destroy the map I want to
delete
all the pointers.

typedef std::map<std::s tring, const T*Table;

void destroy_map ()
{
for (Table::iterato r i = table_.begin(); i != table_.end(); ++i)
{
delete (*i).second;
table_.erase (i);
}
}
this crashes. Does the erase() invalidate the iterator?

This was the code I found on the net (latout fixed a bit)

for (map::iterator itr = myMap.begin(); itr != myMap.end())
{
if(itr->value == something)
myMap.erase(itr ++);
else
itr++;
}

which it is claimed is from Josuttis. I suppose the fact that it
didn't compile should of made me doubt this...
--
Nick Keighley

[Nick Keighley]

On 8 Nov, 10:55, Nick Keighley <nick_keighley_ nos...@hotmail. com>
wrote:

Hi,

I have a map containing pointers. When I destroy the map I want to
delete
all the pointers.

typedef std::map<std::s tring, const T*Table;

void destroy_map ()
{
for (Table::iterato r i = table_.begin(); i != table_.end(); ++i)
{
delete (*i).second;
table_.erase (i);
}

}

this crashes. Does the erase() invalidate the iterator?

This was the code I found on the net (latout fixed a bit)

for (map::iterator itr = myMap.begin(); itr != myMap.end())
{
if(itr->value == something)
myMap.erase(itr ++);
else
itr++;

}

which it is claimed is from Josuttis. I suppose the fact that it
didn't compile should of made me doubt this...

ah! more poking around on the net. erase() does invalidate the
iterator. That's why the net code does a post increment in the erase
call.
So preumably I want:-

void destroy_map ()
{
for (Table::iterato r i = table_.begin(); i != table_.end();)
{
delete (*i).second;
table_.erase (i++);
}
}

since this code is actually in a DTOR and table_ is member variable
would I be better leaving the destruction of the map to the DTOR?

class Symbol_table
{
public:
Table table_;
~Symbol_table() ;
};

Symbol_table::~ Symbol_table()
{
for (Table::iterato r i = table_.begin(); i != table_.end(); ++i)
delete (*i).second;
}

should the for-loop be changed into an algorithm? for_each()?
--
Nick Keighley

[Juha Nieminen]

Nick Keighley wrote:

table_.erase (i++);

No, the standard way is: i = table_.erase(i) ;

Besides, there's no need to individually erase the map elements. Just
free the memory pointed by the elements and then just clear() the map,
given that you are cleaning it completely.

[Daniel T.]

Nick Keighley <ni************ ******@hotmail. comwrote:

I have a map containing pointers. When I destroy the map I want to
delete all the pointers.

typedef std::map<std::s tring, const T*Table;

void destroy_map ()
{
for (Table::iterato r i = table_.begin(); i != table_.end(); ++i)
{
delete (*i).second;
table_.erase (i);
}
}
this crashes. Does the erase() invalidate the iterator?

Yes. Try this instead:

void destroy_map()
{
for ( Table::iterator i = table_.begin(); i != table_.end(); ++i )
{
delete i->second;
i->second = 0; // I don't think this is strictly necessary...
}
table_.clear();
}

or if you want to have fun with algorithms:

typedef map<int, int*Table;

template < typename T >
void delete_second( T& t ) {
delete t.second;
t.second = 0;
}

void destroy_map()
{
for_each( table_.begin(), table_.end(),
&delete_second< Table::value_ty pe);
table_.clear();
}

[Zeppe]

Nick Keighley wrote:

On 8 Nov, 10:55, Nick Keighley <nick_keighley_ nos...@hotmail. com>
wrote:

>Hi,

I have a map containing pointers. When I destroy the map I want to
delete
all the pointers.

typedef std::map<std::s tring, const T*Table;

void destroy_map ()
{
for (Table::iterato r i = table_.begin(); i != table_.end(); ++i)
{
delete (*i).second;
table_.erase (i);
}

}

this crashes. Does the erase() invalidate the iterator?

This was the code I found on the net (latout fixed a bit)

for (map::iterator itr = myMap.begin(); itr != myMap.end())
{
if(itr->value == something)
myMap.erase(itr ++);
else
itr++;

}

which it is claimed is from Josuttis. I suppose the fact that it
didn't compile should of made me doubt this...

ah! more poking around on the net. erase() does invalidate the
iterator. That's why the net code does a post increment in the erase
call.
So preumably I want:-

void destroy_map ()
{
for (Table::iterato r i = table_.begin(); i != table_.end();)
{
delete (*i).second;
table_.erase (i++);
}
}

since this code is actually in a DTOR and table_ is member variable
would I be better leaving the destruction of the map to the DTOR?

It definitely would. And I'll tell you more, that if you use a smart
pointer you can also forget about the deallocation. The bad thing is
that you need to use additional libraries, such as boost.

Then you can have something as:

#include <map>
#include <boost/shared_ptr.h>

typedef boost::shared_p tr<intElementPt r;
typedef std::map<int, ElementPtrTable ;

int main(){
Table t;
t[1] = ElementPtr(new int(1));
t[2] = ElementPtr(new int(2));
}

this will be correctly deleted by the map destructor.

Regards,

Giuseppe

[Daniel T.]

Nick Keighley <ni************ ******@hotmail. comwrote:

ah! more poking around on the net. erase() does invalidate the
iterator. That's why the net code does a post increment in the erase
call.
So preumably I want:-

void destroy_map ()
{
for (Table::iterato r i = table_.begin(); i != table_.end();)
{
delete (*i).second;
table_.erase (i++);
}
}

since this code is actually in a DTOR and table_ is member variable
would I be better leaving the destruction of the map to the DTOR?

class Symbol_table
{
public:
Table table_;
~Symbol_table() ;
};

Symbol_table::~ Symbol_table()
{
for (Table::iterato r i = table_.begin(); i != table_.end(); ++i)
delete (*i).second;
}

should the for-loop be changed into an algorithm? for_each()?

Let the destructor take care of the map, unless you find yourself
putting this kind of loop in all over the place.

I showed in another post how to switch it to using for_each, but I'm not
sure of the utility. I can't think off-hand of any other uses for the
functor, so the only gain would be the removal of the loop.

If, however, you find you have to go through maps like this more than
once in your program, then you should probably write a separate function
for it.

template < typename Map >
void delete_map_ptrs ( Map& m ) {
for ( Map::iterator it = m.begin(); it != m.end(); ++it )
{
delete it->second;
it->second = 0;
}
}

[red floyd]

Juha Nieminen wrote:

Nick Keighley wrote:

> table_.erase (i++);

No, the standard way is: i = table_.erase(i) ;

Besides, there's no need to individually erase the map elements. Just
free the memory pointed by the elements and then just clear() the map,
given that you are cleaning it completely.

Sorry, Juha, but map<>::erase returns void.

[Victor Bazarov]

red floyd wrote:

Juha Nieminen wrote:

>Nick Keighley wrote:

>> table_.erase (i++);

No, the standard way is: i = table_.erase(i) ;

Besides, there's no need to individually erase the map elements.
Just free the memory pointed by the elements and then just clear()
the map, given that you are cleaning it completely.

Sorry, Juha, but map<>::erase returns void.

In C++0x it will return an iterator. Many library implementations
already have that. While in the currect Standard it's 'void', check
your implementation for it as a possible extension.

V
--
Please remove capital 'A's when replying by e-mail
I do not respond to top-posted replies, please don't ask

[James Kanze]

On Nov 8, 2:38 pm, Juha Nieminen <nos...@thanks. invalidwrote:

Nick Keighley wrote:

table_.erase (i++);

No, the standard way is: i = table_.erase(i) ;

Sorry, Juha, but for some silly reason, map<>::erase() doesn't
return an iterator. (This will be corrected in the next version
of the standard, and I think some implementations already
support it, but if you want to be portable and/or standards
conformant, you can't do it.)

Besides, there's no need to individually erase the map elements. Just
free the memory pointed by the elements and then just clear() the map,
given that you are cleaning it completely.

Formally, having a pointer to a deleted object in the map is
undefined behavior. So you really have to either erase or null
the pointer in the map before deleting, something like:
T* tmp = *i ;
table.erase( i ++ ) ;
delete tmp ;
or
T* tmp = NULL ;
std::swap( tmp, *i ) ;
delete tmp ;
(And that's really in code---using swap is definitly stylish.)

Practically, of course, I wouldn't worry about it, and would do
it as you suggest. Or use smart pointers: the case where a
container is the actual owner of an object is one of the cases
where their use is justified. (I'll admit that in my own code,
in almost 20 years of C++, I've only found one case where a
container actually owned something it pointed to. But that's a
different issue.)

--
James Kanze (GABI Software) email:ja******* **@gmail.com
Conseils en informatique orientée objet/
Beratung in objektorientier ter Datenverarbeitu ng
9 place Sémard, 78210 St.-Cyr-l'École, France, [phone]

[Nick Keighley]

On 9 Nov, 09:38, James Kanze <james.ka...@gm ail.comwrote:

<snip>

Formally, having a pointer to a deleted object in the map is
undefined behavior. So you really have to either erase or null
the pointer in the map before deleting

<snip>

Practically, of course, I wouldn't worry about it, and would do
it as you suggest. Or use smart pointers: the case where a
container is the actual owner of an object is one of the cases
where their use is justified. (I'll admit that in my own code,
in almost 20 years of C++, I've only found one case where a
container actually owned something it pointed to. But that's a
different issue.)

well obviously I can't disagree with your experience. But it seemed
not unusual for a container to own an object it pointed to. If you're
newing a lot of similar objects then you need to keep them somewhere.
Why not a container?

The code that prompted my original query was a symbol table.
A definition file is parsed to build the symbol table

typedef std::map<std::s tring,const Type*Table;

This symbol table is then used to decode a stream of data.
The first octet read is looked up to yield the message name,
which is then looked up to find the Type. Type knows how
to parse the rest of the stream.

Other cases might be Calls in a telecommunicati on system.
Drawable items in a drawing program etc.

I'd have thought a container that owned things it pointed to was
very common!
--
Nick Keighley