stl map operator[] bad?

Question

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:)

Answer 1

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.

Answer 2

You are right that map::operator[] has to be used with care, but it can be quite useful: if you want to find an element in the map, and if not there create it:

someClass *&obj = map[x];
if (!obj)
    obj = new someClass;
obj->doThings();

And there is just one lookup in the map. If the new fails, you may want to remove the NULL pointer from the map, of course:

someClass *&obj = map[x];
if (!obj)
    try
    {
        obj = new someClass;
    }
    catch (...)
    {
        obj.erase(x);
        throw;
    }
obj->doThings();

Naturally, if you want to find something, but not to insert it:

std::map<int, someClass*>::iterator it = map.find(x); //or ::const_iterator
if (it != map.end())
{
    someClass *obj = it->second;
    obj->doThings();
}

Answer 3

Claims like "use of operator[] of the map is very bad" should always be a warning sign of almost religious belief. But as with most such claims, there is a bit of truth lurking somewhere. The truth here however is as with almost any other construct in the C++ standard library: be careful and know what you are doing. You can (accidentally) misuse almost everything.

One common problem is potential memory leaks (assuming your map owns the objects):

std::map<int,T*> m;
m[3] = new T;
...
m[3] = new T;

This will obviously leak memory, as it overwrites the pointer. Using insert here correctly isn't easy either, and many people make a mistake that will leak anyways, like:

std::map<int,T*> m;
minsert(std::make_pair(3,new T));
...
m.insert(std::make_pair(3,new T));

While this will not overwrite the old pointer, it will not insert the new and also leak it. The correct way with insert would be (possibly better enhanced with smart pointers):

std::map<int,T*> m;
m.insert(std::make_pair(3,new T));
....
T* tmp = new T;
if( !m.insert(std::make_pair(3,tmp)) )
{
    delete tmp;
}

But this is somewhat ugly too. I personally prefer for such simple cases:

std::map<int,T*> m;

T*& tp = m[3];
if( !tp )
{
    tp = new T;
}

But this is maybe the same amount of personal preference as your code reviewers have for not allowing op[] usage...