C++: How can I stop map's operator[] from inserting bogus values?

Question

My code did the following:

1. Retrieve a value from a map with operator[].
2. Check the return value and if NULL use insert to insert a new element in the map.

Magically, an element with value 0 appeared in the map.

After several hours of debugging I discovered the following: map's operator[] inserts a new element if the key is not found while insert does not change the value if the key exists.

Even if a default constructor for the map value type does not exist the code compiles and operator[] inserts 0.

Is there a way (e.g. some coding convention I could follow from now on) I could have prevented this from hurting me?

Answer 1

operator[] actually returns a Value& so if you are sure you want to insert the element you can do something like:

map<Key, Value*> my_map;

Value& entry = my_map[key];  // insert occurs here, using default constructor.
if (entry == nullptr) entry = my_new_entry;  // just changing the value

Added benefit is that you only lookup in the map once.

Answer 2

Here are several examples of map lookup and insert use cases, where you sometimes want to handle the case of items already existing, seeing what old value was, etc.

class Foo
{
    // Use a typedef so we can conveniently declare iterators
    // and conveniently construct insert pairs
    typedef map<int, std::string> IdMap;
    IdMap id_map;

    // A function that looks up a value without adding anything
    void one(int id)
    {
        IdMap::iterator i = id_map.find(id);

        // See if an entry already exists
        if (i == id_map.end())
            return; // value does not exist

        // Pass the string value that was stored in the map to baz
        baz(i->second);
    }

    // A function that updates an existing value, but only if it already exists
    bool two(int id, const std::string &data)
    {
        IdMap::iterator i = id_map.find(id);

        if (i == id_map.end())
            return false;

        i->second = data;
        return true;
    }

    // A function that inserts a value only if it does NOT already exist
    // Returns true if the insertion happened, returns false if no effect
    bool three(int id, const std::string &data)
    {
        return id_map.insert(IdMap::value_type(id, data)).second;
    }

    // A function that tries to insert if key doesn't already exist,
    // but if it does already exist, needs to get the current value
    void four(int id, const std::string &data)
    {
        std::pair<IdMap::iterator,bool> i =
            id_map.insert(IdMap::value_type(id, data));

        // Insertion worked, don't need to process old value
        if (i->second)
            return true;

        // Pass the id to some imaginary function that needs
        // to know id and wants to see the old string and new string
        report_conflict(id, i->first->second, data);
    }
};

Programmers often do multiple redundant calls to operator[], or a call to find then a redundant call to operator[], or a call to find then a redundant call to insert, out of laziness or ignorance. It is quite easy to efficiently use a map if you understand its semantics.

Answer 3

Is there a way (e.g. some coding convention I could follow from now on) I could have prevented this from hurting me?

This may sound snarky, but: by reading the documentation.

Since what you did is somewhat expected behaviour of the map, there’s not much you can do to guard against it.

One thing you can heed in the future is the following. In your second step, you did something wrong:

Check the return value and if NULL use insert to insert a new element in the map.

This does never work with C++ standard library functions (other than C compatibility functions and new): the standard library doesn’t deal in pointers, least of all null pointers, so checking against NULL (or 0 or nullptr) rarely makes sense. (Apart from that, it wouldn’t make sense for a map’s operator [] to return a pointer in the first place. It obvoiusly returns the element type (or rather, a reference to it)).

In fact, the standard library predominantly uses iterators so if at all, check for iterator validity by comparing against the end() of a container.

Unfortunately, your code (checking against NULL) compiled since NULL is actually a macro that’s equal to 0 in C++ so you can compare it against an integer.

C++11 gets safer by introducing the nullptr keyword which has a distinct type, so comparing it with an integer wouldn’t compile. So this is a useful coding convention: never use NULL, and instead compile with C++11 support enabled and use nullptr.

Answer 4

Latest implementations of std::map also have a .at(const Key& key) member functions which checks for existence of value and returns a std::out_of_range exception if the key has not been found.

http://en.cppreference.com/w/cpp/container/map/at

Answer 5

Even if a default constructor for the map value type does not exist the code compiles

This is definitely wrong. operator[] should fail to compile if a default constructor does not exist. Anything else is an error on the part of your implementation.

Your code should have simply used insert once.

Answer 6

I guess the obvious is to learn that those are indeed the semantics of the indexing operator, so you should not use it to test for element existance in a container.

Instead, use find().

Answer 7

Instead of using the [] operator, call find on your map. This will not insert an entry if no match is found. It returns an iterator to the item found.

Answer 8

Indeed, when you call operator [], if a value at that key is not found, a value-initialized default value is inserted.

If you don't want this to happen, you have to check with find:

if ( mymap.find(myKey) == mymap.end() )
{
    //the key doesn't exist in a map
}

The value returned by operator [] will be NULL only if it's a map to pointers (or types that value-initialized yield 0, but you were pretty specific about NULL).