4

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.

Improve this question
1
  • 1
    Does DisplayHelpAction publicly derive from Action? Is the this->... call located in a non-const member function? There is too little context here. Please fill in the gaps. Commented Feb 15, 2011 at 14:18

3 Answers 3

Reset to default
9

@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

Improve this answer
Sign up to request clarification or add additional context in comments.

1 Comment

+1 for the additional detail and clarification, that actually makes sense since the objective was to avoid having to explicitly free my std::map. I've actually, having asked this, gone with the other suggestion from the original (that is, shared_ptr) since it works with a pure virtual function in Action. The idea wasn't to allow people to instantiate Action, since there isn't any reason for it, but now I'm wondering if that is a problem or not. I might have another question brewing now...!
7

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);
Improve this answer

4 Comments

+1 The missing 'const' in the function's declaration looks like a bug. Maybe someone should raise this issue in the Boost mailing list.
This explains why the key is passed by non-const reference: boost.org/doc/libs/1_45_0/libs/ptr_container/doc/…
I should note though that using this method, boost doesn't like my pure virtual Action though; I have to use a normal virtual. I am assuming there is nothing wrong with this approach.
@sellibitze: it's perfectly intentional (exception safety reason), I've added a lengthy answer that will hopefully explains why.
1

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.

Improve this answer

Comments