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

Question

Following my last question , I have an abstract base class Action that acts as an interface for performing various actions. To implement the abstraction layer, I have an ActionHandler class that stores various actions in it:

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

I was given to understand from the answers to my previous question that boost automatically handles the release of any inserted types of pointers (classes) to this map.

So now I'm trying to insert things derived from Action , this happens in the ActionHandler constructor (ActionHandler :: ActionHandler):

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

Where DisplayHelpAction public subclasses of Action . This 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 that I use:

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

So, as far as I can see, using polymorphism should work here. I do not want to use boost::any , because I do not want this list to contain any pointer. It must match the interface specified by Action or not be there.

So what am I doing wrong?

I can return simply with std::map and have my own destructor delete , so if this cannot be done reasonably, this is not a show stopper. I personally think that shared_ptr with std::map might be better, but, experimenting with this, I now have some reason-not-this-working syndrome.

Answer 1

As you pointed out, the method you are 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-constant reference to std::string . You will have to either make the lvalue key

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

Or use a form that takes a constant reference (there should still be two lines, for exception safety)

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

Answer 2

Answer to

@Cubbi is correct, but does not explain why this was done.

Traditionally, arguments are accepted by const& if they are not inline, so one would expect:

 insert(key_type const&, value*) 

Which naturally allows:

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

But the authors chose a signature:

 insert(key_type&, value*) 

And yes, this is intentional .

The problem is that it is expected that a form using a raw pointer will be used with the built-in new , as you demonstrated in your own example, however there is a security issue with exceptions.

You can read how Sutter will take him to Guru of the Week .

When a function is called in C ++, all its arguments must be evaluated before the call starts, and the order in which the arguments are evaluated is unspecified. Therefore, there is a risk if evaluating the arguments performs memory allocation and another operation (which may throw). In your example:

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

Before making a call, two operations can be performed (in an unspecified order):

• converting "abc" to std::string
• building DerivedAction object in free storage

If the conversion "abc" to std::string thrown out and it was scheduled after memory allocation, then a memory leak occurs because you have no way to free it.

By making the first argument NOT be temporary, they prevented this error. This is not enough (in general), since any function call can be executed and still throw, but it makes you think, right?)

Note: the version that uses auto_ptr by reference does the opposite, they force you to preallocate memory, so the "abc" conversion may throw, no longer take risks, because RAII will provide the proper cleanup

Answer 3

You need to pass the value using auto_ptr :

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

... or create a key before you call insert so that it is passed as a non-constant reference, in which case you do not need an automatic pointer:

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

In the second example, use is enforced using a key job that is not constant in the ptr_map_adapter class. Otherwise, the object that you allocated for insertion on the map may leak if std::string throws an exception.