STL std::map, pass by ref to const and the necessity of const_casting

Question

I have a simple question regarding const_cast and best practices regarding STL containers. Consider the following where class Foo has a private STL std::map from Widget* to int:

Declaration:

#include <map>  
using std::map;

class Widget;

class Foo {
public:
     Foo(int n);
     virtual ~Foo();

     bool hasWidget(const Widget&);

private:
     map<Widget*,int> widget_map;
};

Definition:

#include <map>
#include "Foo.h"
#include "Widget.h"

using std::map;

Foo::Foo(int n)
{
     for (int i = 0; i < n; i++) {
          widget_map[new Widget()] = 1;
     }
}

Foo::~Foo()
{
     map<Widget*, int>::iterator it;
     for (it = widget_map.begin(); it != widget_map.end(); it++) {
          delete it->first;
     }
}

bool Foo::hasWidget(const Widget& w)
{
     map<Widget*, int>::iterator it;
     it = this->widget_map.find(const_cast<Widget*>(&w));
     return ( ! ( it == widget_map.end() ) );
}

Given that hasWidget takes a reference to const as its parameter, the constness needs to be cast away when calling map::find (wiget_map being from Wiget* to int). As far as I can tell, this approach is both sensible and desirable -- but I'm reluctant to accept it as such without feedback from more experienced C++ programmers.

It seems to me that this is one of the few cases of using const_cast appropriately given that we're passing the result of the cast to an STL method. Am I correct?

I realise that other permutations of this question have been posed already (for example, const_cast for vector with object) but none seem to directly address the above.

Thanks in advance.

Answer 1

I think I'm going to fall in the 'subjective and argumentative' through my answer, but I'll give it a shot...

I'm not horrified by the const_cast, but I'm skeptical on your design. The member function hasWidget takes its parameter by const ref : what does this say to the client ? From a client point of view, if I didn't know the implementation, I would probably think that each Widget is compared by value with the parameter. For me, the interface does not reflect the actual behavior, which compares the Widget by address.

For example, the current signature allows a temporary Widget to be passed, although the return value could never be true in this case. I would personally change the signature to (note that I added a const) :

bool hasWidget(const Widget *) const;

Answer 2

Why not use a map<const Widget*,int>? You don't seem to ever modify the Widget pointed to by any of the keys in your map.

Assuming there's a good reason then yes, I think you're right. When calling code which is guaranteed not to modify the referand of the pointer, it's safe to cast away const. Because of the way containers of pointers are templated, none of their functions ever directly modify that referand, but if the contained type were a const pointer, then users wouldn't be able to modify the referand either (without a const cast). It's certainly safer to cast away const before searching, than to cast away const before modifying, if it must be one of the two...

Btw, hasWidget would be shorter if you use count rather than find. It's also marginally const-safer in general (not in this case) to use count, because find with this const_cast returns an iterator that could be used to modify the Widget, whereas count doesn't. So you don't have to worry what happens to the return value of count. Obviously here that return value is entirely under control anyway.

Answer 3

Yes, that's a reasonable use of const_cast<>. You should consider making hasWidget const.

Answer 4

Why not change hasWidget to take a Widget*? The interface is dodgy at the moment, because it implies that you're looking for Widgets by value in the underlying map, when you're actually looking for them by address. The method should also be const, I reckon:

bool hasWidget(Widget *) const;

Answer 5

A map with a key where the key is pointer is unwieldy - the only way to look it up is to have the same pointer. For this to work, you have to guarantee that the the hasWidget method will get called with an object that has the same address!

Surely you should implement Widget properly such that it has the correct operators overloaded to act as a key in a std::map! In your map, you can then simply have:

std::map<Widget, int>

And then your find doesn't need a const_cast!

Answer 6

This looks clunky to me. Identifying objects by their physical addresses is quite "special", admittedly it's unique, but it's weird too.

I would strongly consider reversing the map:

std::map<Widget::Id, Widget*>

where Widget::Id could simply be an int or similar.

There would not be any issue with the const-ness then.

To delve deeper, you could also have a look at the Boost Pointer Container library:

boost::ptr_map<Widget::Id, Widget>

which would alleviate the memory management issues.