std::map crash

[Thomas Wegener]

I create the items for the std:map with new and before I clear the map I
delete all items, deleteSecond() in the following sample. Unfortunately my
application crash sometimes during map::clear or during new allocation of
items. I assume somthing is wrong in my template.

Somebody any idea what can be wrong?

template <class S, class T>
class MyMap : public std::map< S*, T*, std::less<S*> >
{
public:
~MyMap() {
deleteSecond();
}
void deleteSecond() {
for( iterator i = begin(); i != end(); ++i ) {
delete (*i).second;
(*i).second = 0;
}
}
void clear() {
deleteSecond();
std::map< S*, T*, std::less<S*> >::clear();
}
T * get( const S * key ) {
iterator i = find( const_cast<S*>( key ) );
return end() == i ? 0 : (*i).second;
}
bool put( S * a, T * b ) {
return insert( value_type( a, b ) ).second;
}
};

Thanks Thomas

[Tom Widmer [VC++ MVP]]

Post by Thomas Wegener
I create the items for the std:map with new and before I clear the map I
delete all items, deleteSecond() in the following sample. Unfortunately my
application crash sometimes during map::clear or during new allocation of
items. I assume somthing is wrong in my template.
Somebody any idea what can be wrong?
template <class S, class T>
class MyMap : public std::map< S*, T*, std::less<S*> >
{
~MyMap() {
deleteSecond();
}
void deleteSecond() {
for( iterator i = begin(); i != end(); ++i ) {
delete (*i).second;
(*i).second = 0;
}
}
void clear() {
deleteSecond();
std::map< S*, T*, std::less<S*> >::clear();
}
T * get( const S * key ) {
iterator i = find( const_cast<S*>( key ) );
return end() == i ? 0 : (*i).second;
}
bool put( S * a, T * b ) {
return insert( value_type( a, b ) ).second;
}
};

This is inherintly dodgy - elements can be removed from your map without
the value being deleted, by calling map::erase (this is due to the
underlying problem of deriving publically from std::map). Also, your map
appears to take ownership of value pointers passed to it, but not of key
pointers, but this is not very explicit. I think a better solution would
be use to smart pointers - take a look at shared_ptr from www.boost.org.
Also, take a look at the pointer containers there - ptr_map<S*, T> may
do what you want.

Tom

[Igor Tandetnik]

Post by Thomas Wegener
I create the items for the std:map with new and before I clear the
map I delete all items, deleteSecond() in the following sample.
Unfortunately my application crash sometimes during map::clear or
during new allocation of items. I assume somthing is wrong in my
template.
Somebody any idea what can be wrong?

Do you always insert items allocated with new? Do you ever delete these
items once inserted, besides from MyMap's destructor or clear() methods?
From your description, it is obvious you have heap corruption.

[Doug Harrison [MVP]]

Post by Thomas Wegener
I create the items for the std:map with new and before I clear the map I
delete all items, deleteSecond() in the following sample. Unfortunately my
application crash sometimes during map::clear or during new allocation of
items. I assume somthing is wrong in my template.
Somebody any idea what can be wrong?
template <class S, class T>
class MyMap : public std::map< S*, T*, std::less<S*> >
{
~MyMap() {
deleteSecond();
}
void deleteSecond() {
for( iterator i = begin(); i != end(); ++i ) {
delete (*i).second;
(*i).second = 0;
}
}
void clear() {
deleteSecond();
std::map< S*, T*, std::less<S*> >::clear();
}
T * get( const S * key ) {
iterator i = find( const_cast<S*>( key ) );
return end() == i ? 0 : (*i).second;
}
bool put( S * a, T * b ) {
return insert( value_type( a, b ) ).second;
}
};

This can be written a little more simply (and correctly, based on standard
conformance issues and your usage patterns inferred from your const_cast)
as follows:

template <class S, class T>
class MyMap : public std::map<const S*, T*>
{
private:

typedef std::map<const S*, T*> base;

public:
~MyMap() {
deleteSecond();
}
void deleteSecond() {
for (typename base::iterator i = base::begin(); i != base::end(); ++i)
{
delete i->second;
i->second = 0;
}
}
void clear() {
deleteSecond();
base::clear();
}
T * get( const S * key ) {
typename base::iterator i = base::find(key);
return base::end() == i ? 0 : i->second;
}
bool put(const S* a, T* b) {
return base::insert( typename base::value_type( a, b ) ).second;
}
};

A couple of things about this design concern me, and they both stem from
public inheritance of std::map:

1. While your dtor may be correct, assignment and copy construction are
not. You'll have to add them, or more likely, disable them by declaring
them private without defining them.

2. You're hiding map::clear with MyMap::clear, which is always bad under
public inheritance. The problem is map::clear is not virtual, so calling
clear through a pointer or reference to base will not have the same effect
as calling it through a pointer or reference to MyMap.

I'd suggest not deriving publicly from std::map or any other STL container,
because none of them have virtual functions, and none are meant to be
publicly derived from. Your best bet is to use containment instead of
inheritance.

Concerning your problem, (1) could be the culprit, because if you're
copying MyMap objects, you may end up double-deleting the pointers they
contain. If you're not copying MyMap objects, then I'd wonder about
ownership issues, including the good old separate CRTs (and heaps) that
result in multiple module scenarios when all the modules don't link to the
same CRT DLL.

P.S. Note that map::insert does not update an existing item in the map, so
your "put" function leaves the map unchanged when "a" already occurs in the
map.

[Thomas Wegener]

Thanks for all responses.
- I need this map only for temporary purposes. So I don't copy, assign, or
delete any item during the live time.
- Nevertheless all your objections are right and I will change the class
design from STL inheritance to STL containment and declare the asign
operators, ... as private.

Thanks Thomas

[Simon Trew]

Doug,

I liked your finesse about deriving 'publicly'.

Often it is stated that one should not "ever" derive from the Standard
Library container classes (or sometimes just from any Standard Library
classes), but of course it is OK if you know what you are doing, and are
deriving for your own ends *and* relying only on the published semantics and
not just implementation details. Which is tantamount to saying, one can
derive privately rather than provide pointless wrapping shims for no good
purpose. Protectedly, well protectedly...

Best,

S.

[Doug Harrison [MVP]]

Post by Simon Trew
Doug,
I liked your finesse about deriving 'publicly'.
Often it is stated that one should not "ever" derive from the Standard
Library container classes (or sometimes just from any Standard Library
classes), but of course it is OK if you know what you are doing, and are
deriving for your own ends *and* relying only on the published semantics and
not just implementation details. Which is tantamount to saying, one can
derive privately rather than provide pointless wrapping shims for no good
purpose. Protectedly, well protectedly...

Yes, while the absence of virtual functions in STL containers makes them
poor candidates for public inheritance, it also makes them good candidates
for private inheritance. I used to use private inheritance a lot, and even
recommended it as a more convenient form of containment when the derived
class ISA type of the base class from the perspective of the class
designer, but I started to back away from it when I learned it's always
possible to override a virtual function, even one declared in a base class
that's inherited privately. It isn't legal for standard library
implementations to add new virtual functions to standard classes, so that
shouldn't be a concern for STL classes. So I'd say privately inheriting
from STL containers is still a good option, one that can save you some
work, especially when you want to provide a lot of the base class interface
to users, i.e. you can write using-declarations instead of forwarding
functions for most everything except ctors and assignment operators.
However, inside the derived class, especially when it's a class template
like the OP's that derives from a template that isn't fully specialized,
it's more of a wash, because the lookup rules require you to qualify
everything you want to use from the base class, either with base::begin()
like I used in my previous message, or perhaps this->begin(). Containment
as realized by member data does have fewer such intricacies.