Return value goes bad with std::map and boost::ptr_map templates and inheritance

Question

At the company I work at we created a class called 'RestrictedMap'. This provides the same interface as a regular std::map but will not allow you to use the [] operator. Some other functions have been provided to work with the class comfortably. Internally the class wraps an std::map.

I'm now trying to create a similar class that does the same for a boost::ptr_map, called 'RestrictedPointerMap'. For this I've created RestrictedMapBase that accepts, as a template argument, the type of map it should wrap and contains most of the implementation. Two classes derive of it and specify the type of map to be wrapped:

RestrictedMap that does the same as what we used to have and wraps std::map
and RestrictedPointerMap that is new and wraps boost::ptr_map.

Here's the code, I haven't simplified the class for completeness but I will name the relevant functions later on.

RestrictedMap.h

    #pragma once

    #include <boost/ptr_container/ptr_map.hpp>
    #include <boost/static_assert.hpp>
    #include <boost/assign/list_of.hpp>
    #include <boost/foreach.hpp>
    #include <map>

    /**
    * Class that has the benefits of a map, but does not add an entry if it does not exists. 
    * Do not use RestrictedMapBase directly but use one of the derived classes (RestrictedMap or RestrictedPointerMap).
    */
    template <typename MAP>
    class RestrictedMapBase
    {
    public:
       RestrictedMapBase(const MAP& map): 
          m_map(map)
       {}

       template<class InputIterator>
       RestrictedMapBase(InputIterator first, InputIterator last): 
          m_map(first, last)
       {}

       RestrictedMapBase()
       {}

       /************************************************************************/
       /* std::map interface                                                   */
       /************************************************************************/

       typedef typename MAP::iterator iterator;
       typedef typename MAP::const_iterator const_iterator;
       typedef typename MAP::value_type value_type;
       typedef typename MAP::key_type key_type;
       typedef typename MAP::mapped_type mapped_type;
       typedef typename MAP::size_type size_type;

       iterator begin() { return m_map.begin(); }
       iterator end() { return m_map.end(); }
       const_iterator begin() const { return m_map.begin(); }
       const_iterator end() const { return m_map.end(); }
       bool empty() const { return m_map.empty(); }
       size_type size() const { return m_map.size(); }
       iterator find(const key_type& key) { return m_map.find(key); } 
       const_iterator find(const key_type& key) const { return m_map.find(key); }
       void clear() { m_map.clear(); }
       void erase(iterator where) { m_map.erase(where); }

       bool operator==(const typename RestrictedMapBase<MAP>& other) const { return m_map == other.m_map; }
       bool operator!=(const typename RestrictedMapBase<MAP>& other) const { return m_map != other.m_map; }
       bool operator<(const typename RestrictedMapBase<MAP>& other) const { return m_map < other.m_map; }

       /************************************************************************/
       /* extra                                                                */
       /************************************************************************/

       void erase(const key_type& key)
       {
          iterator iter(find(key));
          assert(found(iter));
          erase(iter);
       }

       void eraseIfExists(const key_type& key)
       {
          m_map.erase(key);
       }

       bool exists(const key_type& key) const
       {
          return found(find(key));
       }

       mapped_type& getValue(const key_type& key)
       {
          return const_cast<mapped_type&>(static_cast<const RestrictedMapBase<MAP>&> (*this).getValue(key));
       }

       const mapped_type& getValue(const key_type& key) const
       {
          const_iterator iter(find(key));
          assert(found(iter));
          return getData(iter);
       }

       mapped_type getValueIfExists(const key_type& key) const
       {
          BOOST_STATIC_ASSERT(boost::is_pointer<mapped_type>::value);
          const_iterator iter(find(key));
          if (found(iter)) {
             return getData(iter);
          } else {
             return 0;
          }
       }

       void setValue(const key_type& key, const mapped_type& value)
       {
          iterator iter(find(key));
          assert(found(iter));
          setData(iter, value);
       }

       void add(const key_type& key, const mapped_type& value)
       {
          assert(!exists(key));
          insert(key, value);
       }

       void add(const RestrictedMapBase<MAP>& mapToAdd)
       {
          BOOST_FOREACH(value_type element, mapToAdd.m_map)
          {
             add(element.first, element.second);
          }
       }

       void addOrReplace(const key_type& key, const mapped_type& value)
       {
          iterator iter(find(key));
          if (found(iter)) {
             setData(iter, value);
          } else {
             insert(key, value);
          }
       }

       mapped_type* addDefaultConstructed(const key_type& key)
       {
          assert(!exists(key));
          return &m_map[key];
       }

    private:
       bool found(const const_iterator& iter) const 
       {
          return iter != end();
       }

       const mapped_type& getData(const const_iterator& iter) const
       {
          return const_cast<const mapped_type&>(iter->second);
       }

       mapped_type& getData(const iterator& iter)
       {
          return const_cast<mapped_type&>(static_cast<const RestrictedMapBase<MAP>&>(*this).getData(iter));
       }

       void setData(const iterator& iter, const mapped_type& value)
       {
          getData(iter) = value;
       }

       virtual void insert(const key_type& key, const mapped_type& value) = 0;

    protected:
       MAP& getMap()
       {
          return m_map;
       }

    private:
       MAP m_map;
    };

    template <typename KEYTYPE, typename DATATYPE>
    class RestrictedMap: public RestrictedMapBase<std::map<KEYTYPE, DATATYPE> >
    {
    public:
       RestrictedMap(const std::map<typename KEYTYPE, typename DATATYPE>& map): RestrictedMapBase(map)
       {}

       template<class InputIterator>
       RestrictedMap(InputIterator first, InputIterator last): RestrictedMapBase(first, last)
       {}

       RestrictedMap()
       {}

       virtual void insert(const KEYTYPE& key, const DATATYPE& value)
       {
          getMap().insert(std::make_pair(key, value));
       }
    };

    template <typename KEYTYPE, typename DATATYPE>
    class RestrictedPointerMap: public RestrictedMapBase<boost::ptr_map<KEYTYPE, DATATYPE> >
    {
    public:
       RestrictedPointerMap(const boost::ptr_map<typename KEYTYPE, typename DATATYPE>& map): RestrictedMapBase(map)
       {}

       template<class InputIterator>
       RestrictedPointerMap(InputIterator first, InputIterator last): RestrictedMapBase(first, last)
       {}

       RestrictedPointerMap()
       {}

       virtual void insert(const KEYTYPE& key, DATATYPE* const& value)
       {
          /* boost::ptr_map::mapped_type does not equal the DATATYPE template parameter passed to it. Therefore this 
           * functions signature *looks* different from the RestrictedMapBase::insert signature */
          getMap().insert(key, std::auto_ptr<DATATYPE>(value));
       }
    };

This works mostly, except when I want to call getValue on a RestrictedPointerMap. The function getData returns the correct value but after that it goes wrong in the getValue function. It returns a wrong pointer (as in the pointer is wrong).

Here is some code that reproduces the issue:

TestClass.h

    #pragma once

    class SomeClass
    {
    public:
       SomeClass();
       virtual ~SomeClass();
    };

TestClass.cpp

    #include "stdafx.h"
    #include "TestClass.h"
    #include <iostream>

    SomeClass::SomeClass()
    {
       std::cout << "TestClass[" << this << "] created." << std::endl;
    }

    SomeClass::~SomeClass()
    {
       std::cout << "TestClass[" << this << "] deleted." << std::endl;
    }

TestRestrictedPtrMap.cpp (main)

    #include "stdafx.h"

    #include "RestrictedMap.h"
    #include "TestClass.h"
    #include <boost/foreach.hpp>

    int _tmain(int argc, _TCHAR* argv[])
    {
       typedef RestrictedPointerMap<int, SomeClass> MapType;
       MapType theMap;
       theMap.add(1, new SomeClass());
       theMap.add(2, new SomeClass());

       BOOST_FOREACH(MapType::value_type mapEntry, theMap) {
          std::cout << mapEntry.first << " = " << mapEntry.second << std::endl;
       }

       SomeClass* oneClass = theMap.getValue(1);
       std::cout << oneClass << std::endl;
       SomeClass* twoClass = theMap.getValue(2);
       std::cout << twoClass << std::endl;

       std::cin.get();
        return 0;
    }

The output of this is:

    TestClass[0078A318] created.
    TestClass[0078A448] created.
    1 = 0078A318
    2 = 0078A448
    0018FBD4
    0018FBD4
    TestClass[0078A318] deleted.
    TestClass[0078A448] deleted.

I have no clue why it goes wrong. As far as I know the return value goes bad by magic.

Thanks in advance for any help,

Tom

What's with the `static_cast` in `return const_cast(static_cast&> (*this).getValue(key));`? That looks mighty suspicious to me. — David Schwartz, Nov 23 '12 at 17:01
@DavidSchwartz: It's a typical strategy to avoid duplicating the work between `const` and non-`const` versions. You can do it either way, but having the non-`const` version call the `const` version (like shown here) is better since the `const` version will not accidentally modify the object. I think it even appears in Meyers' Efficient C++. — Matthieu M., Nov 23 '12 at 17:26
This code is like Frenchto my eyes :) getting rusty it seems. Anyway, for your goal, why not template class RestrictedMapBase : public MAP and then just hide the [] operator somehow. Perhaps by making it private, if possible. — kellogs, Nov 23 '12 at 17:33
@kellogs: the code sure is weird, but composition should be preferred to inheritance when possible, and this is such case. — Matthieu M., Nov 23 '12 at 17:36
@Tekar: Unfortunately, your code is erroneous (probably due to the number of microsoftisms that plagues it), so it's pretty hard to try and reproduce locally. The strange combination of templates and virtual methods makes me think the original author was not too C++ savvy (your code is wide open to object slicing and the destructor of the base class is not protected); unfortunately this is not a good indication of the bug you are facing. — Matthieu M., Nov 23 '12 at 17:39
@MatthieuM.: I'm not complaining about the `const_cast` but about the `static_cast`. — David Schwartz, Nov 23 '12 at 17:40
@DavidSchwartz: How would you make `*this` `const` so that you can invoke the right overload of `getValue` ? Personally, I use a variable `Type const& me = *this; return const_cast(me.get());` but a `static_cast` is the right tool for the job if you wish to inline. — Matthieu M., Nov 23 '12 at 18:02
The whole idea of `RestrictedMap` seems wrong. If you don't want `operator[]` just don't use it. If you want to extend it use free functions. — GManNickG, Nov 23 '12 at 18:25

ecatmur · Accepted Answer · 2012-11-23T19:33:37.127

You've got a dangling reference.

When you dereference a boost::ptr_map<Key, T>::iterator it constructs on-the-fly a boost::ptr_container_detail::ref_pair<Key, T *> initialised from the actual underlying iterator (a std::map<Key, void *>::iterator). This means that the T *& (or const T *&) returned from getData is referencing a member of a local temporary (the second member of iter->second):

   const mapped_type& getData(const const_iterator& iter) const
   {
      return const_cast<const mapped_type&>(iter->second); // reference to a temporary
   }
                                            ^^^^^^ *iter is a temporary value

This differs from a normal std::map, where *iter gives a reference to the value subobject of the node in the map's binary tree.

There's no easy solution without significantly changing your interface, as there is no actual T * object anywhere in memory to take a reference to. You might do better to change the signature of your RestrictedPointerMap to return the T mapped values by value-pointer or even by direct reference:

T *getValue(const key_type& key);               // not T *&
const T *getValue(const key_type& key) const;   // not const T *const &
// or
T &getValue(const key_type& key);               // not T *&
const T &getValue(const key_type& key) const;   // not const T *const &

Return value goes bad with std::map and boost::ptr_map templates and inheritance

1 Answers1