views:

940

answers:

6

I'm going to extend the existing std::map class and add a new function to it:

template<typename key_type, typename value_type>
class CleanableMap : public Cleanable, public std::map<key_type, value_type> 
{
    CleanableMap(const CleanableMap& in); //not implemented
    CleanableMap& operator=(const CleanableMap& in); //not implemented
public:
    CleanableMap() {}
    CleanableMap(const std::map<key_type, value_type>& in) { *this = in; }
    virtual ~CleanableMap() {}
    std::map<key_type, value_type>& operator=(const std::map<key_type, value_type>& in)
    {
     *((std::map<key_type, value_type>*)this) = in;
     return *this;
    }
};

I've got a copy constructor and assignment operator such that I can simply assign an existing std::map of the same type to my new map:

CleanableMap<DWORD, DWORD> cm;
std::map<DWORD, DWORD> stdm;
cm = stdm;

The problem is, the compiler is complaining with an error that doesn't make sense -- I've explicitly coded for what it's complaining about:

1>c:\dev\proj\commonfunc.cpp(399) : error C2679: binary '=' : no operator found which takes a right-hand operand of type 'std::map<_Kty,_Ty>' (or there is no acceptable conversion)
1>        with
1>        [
1>            _Kty=DWORD,
1>            _Ty=DWORD
1>        ]
1>        c:\dev\proj\templates.h(245): could be 'CleanableMap<key_type,value_type> &CleanableMap<key_type,value_type>::operator =(const CleanableMap<key_type,value_type> &)'
1>        with
1>        [
1>            key_type=DWORD,
1>            value_type=DWORD
1>        ]
1>        c:\dev\proj\templates.h(250): or       'std::map<_Kty,_Ty> &CleanableMap<key_type,value_type>::operator =(const std::map<_Kty,_Ty> &)'
1>        with
1>        [
1>            _Kty=unsigned long,    <--- where did it come up with that?
1>            _Ty=std::pair<const DWORD,DWORD>,  <--- where did it come up with that?
1>            key_type=DWORD,
1>            value_type=DWORD
1>        ]
1>        while trying to match the argument list '(CleanableMap<key_type,value_type>, std::map<_Kty,_Ty>)'
1>        with
1>        [
1>            key_type=DWORD,
1>            value_type=DWORD
1>        ]
1>        and
1>        [
1>            _Kty=DWORD,
1>            _Ty=DWORD
1>        ]

There 'could be' it mentions on line 245 doesn't make sense -- there is no assignment operator like that (well, it's private. Removing it completely doesn't change anything).

The 'could be' it mentions on line 250 is the assignment operator that I defined, but it was somehow deduced some other non-matching template types. Where did it get those??

Help!!! :)

+12  A: 

Std:;map is not intended to be extended by derivation. If you want to add new functionality, do it with stand-alone functions.

anon
I'm really just doing this so I can call a method when the map is constructed and destructed. If I can do this, I can go through and replace std::map with my new CleanableMap and not have to change anything else. So no new data members.And even though it wasn't meant to be derived from, what if I was deriving from some other templated class and got this same error? :)
DougN
@DougN, the problem is you simply cannot guarantee your destructor will be called when the type is destructed. The reason why is that std::map does not have a virtual destructor. See my answer for a concrete scenario of where this will happen.
JaredPar
It doesn't matter whether thare are new data members - you are using std::map in a way its designers did not intend.
anon
You may be interested in using ScopeGuard (create a ScopeGuard object after your map, both the map an the ScopeGuard object will go out of scope at the same time and be destroyed in reverse order; allowing your ScopeGuard object to clean up your map). The ScopeGuard article can be found at http://www.ddj.com/cpp/184403758 .
Max Lybbert
+14  A: 

Adding onto Neil's answer.

One concrete reason you should not be deriving from std::map is that it does not have a virtual destructor. This means you simply cannot guarantee any resources you allocate as a part of your map will be freed during the destruction of your implementation via a std::map pointer.

std::map<int,int>* pMap = GetCleanableMap();
...
delete pMap; // does not call your destructor
JaredPar
aaaahhh, that's a deadly bug. thanks for pointing it out!
DougN
not necessarily. You can prevent this with access control. See my answer below.
Potatoswatter
+2  A: 

And one last tip to add to those:

When it comes to extending (anything, really), it's almost always better to do it with a wrapper class than inheritance.

GMan
I disagree with you there. If the thing was designed for extension by derivation, then you should derive, so that you can use the new things in place of the old. This is not one of those cases.
Michael Kohne
+2  A: 

What you are running into is probably an artifact of the exact implementation of std::map in your environment. Since std::map was never intended to be derived from, implementations often have either odd specializations or weird-looking internals that server to make them much faster, but which lead to lots of strange errors if you try to derive from them.

Alternately, there might be some oddities with your other base class - what does 'Cleanable' look like? If you remove the 'Cleanable' base (as a test) does everything else compile? Might there be some weird internal conflict with their names?

Michael Kohne
+1  A: 
std::map<key_type, value_type>& operator=(const std::map<key_type, value_type>& in)
{
    *((std::map<key_type, value_type>*)this) = in;
    return *this;
}

This sourcecode tries to convert your CleanableMap to std::map. *this is of the type CleanableMap& but the method returns std::map&.

The correct signature of your = operator should be:

CleanableMap &operator=(const std::map<key_type, value_type> &in);

But if you only want to add a "clean" feature to std::map, why don't you just use a function to do the job?

template<typename K, typename V> void clean(std::map<K, V> &map);

To derive from a class with non-virtual destructor should be avoided, as said by JaredPar.

Use a wrapper or function.

neverlord
+3  A: 

A lot of people are dissing inheritance from STL objects. You can make the process safer by using access protection, though: use map as a private base. Here's a demonstration.

#include <iostream>
#include <map>

using namespace std;

struct cmap : private map<int, int> {
        cmap() : map<int, int>() { };

        using map<int, int>::operator[]; // repeat for other methods you'd like
};

int main( int argc, char **argv ) {
        cmap my_map;

        my_map[1] = 2; // behaves like a map

        cerr << my_map[1] << " " << my_map[2] << endl;

        // map<int, int> *bad_map = &my_map; this does not compile

        return 0;
}
Potatoswatter