C++ – stl map operator[] bad

cmapoperatorsstl

My code reviewers has pointed it out that the use of operator[] of the map is very bad and lead to errors:

map[i] = new someClass;    // potential dangling pointer when executed twice

Or

if (map[i]==NULL) ...      // implicitly create the entry i in the map

Although I understand the risk after reading the API that the insert() is better of since it checks for duplicate, thus can avoid the dangling pointer from happening, I don't understand that if handled properly, why [] can not be used at all?

I pick map as my internal container exactly because I want to use its quick and self-explaining indexing capability.

I hope someone can either argue more with me or stand on my side:)

Best Answer

The only time (that I can think of) where operator[] can be useful is when you want to set the value of a key (overwrite it if it already has a value), and you know that it is safe to overwrite (which it should be since you should be using smart pointers, not raw pointers) and is cheap to default construct, and in some contexts the value should have no-throw construction and assignment.

e.g. (similar to your first example)

std::map<int, std::unique_ptr<int>> m;
m[3] = std::unique_ptr<int>(new int(5));
m[3] = std::unique_ptr<int>(new int(3)); // No, it should be 3.

Otherwise there are a few ways to do it depending on context, however I would recommend to always use the general solution (that way you can't get it wrong).

Find a value and create it if it doesn't exist:

1. General Solution (recommended as it always works)

std::map<int, std::unique_ptr<int>> m;
auto it = m.lower_bound(3);
if(it == std::end(m) || m.key_comp()(3, it->first))
   it = m.insert(it, std::make_pair(3, std::unique_ptr<int>(new int(3)));

2. With cheap default construction of value

std::map<int, std::unique_ptr<int>> m;
auto& obj = m[3]; // value is default constructed if it doesn't exists.
if(!obj)
{
   try
   {
      obj = std::unique_ptr<int>(new int(3)); // default constructed value is overwritten.
   }
   catch(...)
   {
      m.erase(3);
      throw;
   }
}

3. With cheap default construction and no-throw insertion of value

std::map<int, my_objecct> m;
auto& obj = m[3]; // value is default constructed if it doesn't exists.
if(!obj)
   obj = my_objecct(3);

Note: You could easily wrap the general solution into a helper method:

template<typename T, typename F>
typename T::iterator find_or_create(T& m, const typename T::key_type& key, const F& factory)
{
    auto it = m.lower_bound(key);
    if(it == std::end(m) || m.key_comp()(key, it->first))
       it = m.insert(it, std::make_pair(key, factory()));
    return it;
}

int main()
{
   std::map<int, std::unique_ptr<int>> m;
   auto it = find_or_create(m, 3, []
   {
        return std::unique_ptr<int>(new int(3));
   });
   return 0;
}

Note that I pass a templated factory method instead of a value for the create case, this way there is no overhead when the value was found and does not need to be created. Since the lambda is passed as a template argument the compiler can choose to inline it.

Related Topic