C++ boost::ptr_map with abstract base class causing insertion issues

Question

Following on from my last question I have an abstract base class Action which acts as an interface for executing various different actions. In order to implement an abstraction layer, I have an ActionHandler class that stores various actions in this:

class ActionHandler
{
public:
    ActionHandler();
    ~ActionHandler();
    Action& getAction(std::string ActionString);
private:
    boost::ptr_map<std::string, vx::modero::Action> cmdmap;
};

I'm given to understand from the responses to my previous question that boost automatically handles freeing of any inserted pointer types (classes) into this map.

So, I now try inserting things derived from Action, this happens in the constructor of ActionHandler (ActionHandler::ActionHandler):

ActionHandler::ActionHandler()
{
    this->cmdmap.insert("help", new DisplayHelpAction());
};

Where DisplayHelpAction publicly subclasses Action. Doing so causes this error:

error: no matching function for call to ‘boost::ptr_map<std::basic_string<char>, 
Action>::insert(const char [5], DisplayHelpAction*)’

Now, from here the method I'm using is:

std::pair<iterator,bool>  insert( key_type& k, T* x );

So as far as I can see, using polymorphism here should work. I don't want to use boost::any because I don't want this list to contain any type of pointer. It should conform to the interface specified by Action or not be there.

So, what am I doing wrong?

I can fall back on simply using std::map and have my destructor delete, so if this can't be reasonably achieved it isn't a show stopper. I personally think shared_ptr with std::map might be better, but having experimented with this, I now have so-why-doesn't-this-work syndrome.

Answer 1

@Cubbi's answer is correct, but doesn't explain why it has been done so.

Traditionally, the arguments are taken by const&, unless they are built-in, so one would naturally expect:

insert(key_type const&, value*)

Which would naturally allow:

someMap.insert("abc", new DerivedAction());

But the authors chose the signature:

insert(key_type&, value*)

And yes, it is intentional.

The problem is that the form taking a raw pointer is expected to be used with an inlined new, as you demonstrated in your own example, however there is an exception safety issue.

You can read Sutter's take on it at Guru Of The Week.

When a function is called in C++, all its arguments should be evaluated before the call begins, and the order of evaluation of the arguments is unspecified. There is a risk, therefore, if the evaluation of arguments performs a memory allocation AND another operation (that may throw). In your example:

insert("abc", new DerivedAction());

// equivalent to

insert(std::string("abc"), new DerivedAction());

There are two operations to be done (in unspecified order) before the call can be executed:

• the conversion of "abc" into a std::string
• the construction on the free-store of a DerivedAction object

If the conversion of "abc" into a std::string throws, and it was scheduled after the memory allocation, then memory is leaked, because you have no way to free it.

By forcing the first argument NOT to be a temporary, they prevented this mistake. It is not sufficient (in general) since any function call could be performed and still throw, but it does make you think, does it not :) ?

Note: the version taking an auto_ptr by reference does so the other way around, they force you to allocate the memory beforehand, so the conversion of "abc" possibly throwing is not a risk any longer because RAII will ensure proper clean-up

Answer 2

As you pointed out, the method you're calling is

std::pair<iterator,bool>  insert( key_type& k, T* x );

but the first argument is not of a type that can be bound to a non-const reference to std::string. You would have to either make the key an lvalue

std::string key = "help";
cmdmap.insert(key, new DisplayHelpAction());

Or use the form that takes a const reference (still has to be two lines, for exception safety)

std::auto_ptr<DisplayHelpAction> ptr(new DisplayHelpAction());
cmdmap.insert("help", ptr);

Answer 3

You need either to pass value using auto_ptr:

std::auto_ptr<Action> action (new DisplayHelpAction ());
cmdmap.insert ("help", action);

... or create key before you invoke insert so that it will be passes as non-const reference, in which case you don't need auto pointer:

std::string key ("help");
cmdmap.insert (key, new DisplayHelpAction());

In the second example, usage is enforced by making key reference non-constant in the ptr_map_adapter class. Otherwise, the object you have allocated to insert to the map could leak if std::string throws exception.