How to prevent unordered_map.find() being compared to a wrong end iterator?

Question

We have just found a bug in our code, that goes something like this:

class foo {
    unordered_map<int,string> m_someMap;
public:
     void stuff() {
        unordered_map<int, string> someMap;
        ...
        auto it = someMap.find(x);
        if (it != m_someMap.end()) { // oops! comparing to the end() of a wrong map!
             // found x in the map, use it->second here 
        }
     }
}

The obvious problem here is that checking if find() succeeded requires naming the map twice, and if those two occurrences went out of sync, user beware.

What is the best practice to prevent this? In other words, how can I check that an item is present in a map without having to say the map's name twice?

I wish map iterators were convertible to bool, so I could write simple if (it)..., but they aren't.

Any other ideas? safe_find() template function that returns an optional<unordered_map<int,string>::iterator>>? Some magic form the modern standard library?

Answer 1

Do you need to check only for existence of given key, or do sth with the found element too? There's a plethora of options, depending on what is to be achieved and lanaguage version in use.

1. Custom template method that returns a pointer, optional, you name it. That's pretty self-explanatory. Hard to give an exhaustive answer here, can be implemented in many ways. I would probably opt for sth like this:

#include <map>
#include <optional>
#include <functional>
#include <type_traits>

template<typename M, typename K>
auto mapFind(M& map, const K& key)
{
    using MappedType = 
        std::conditional_t<std::is_const_v<M>,
            const typename M::mapped_type,
            typename M::mapped_type>;
    using ReturnType = std::optional<std::reference_wrapper<MappedType>>;
    auto found = map.find(key);
    if (found == map.end()) {
        return ReturnType{std::nullopt};
    }
    return ReturnType{found->second};
}

https://godbolt.org/z/q1qbxKMs7

2. map::containsmapObject.contains(key); from C++20. Returns a bool.
3. map::countmapObject.count(key) > 0; pre C++20 way of point two. Slightly uglier, but works.
4. map::attry { mapObject.at(key); } catch (const std::out_of_range& ex) {//handle ex here} Exception based, therefore oftentimes disliked or discouraged, but works.

Answer 2

You can create a free template function for to avoid any accidents:

template<typename K, typename V>
static bool exists_in_unordered_map(const std::unordered_map<K, V>& m, const K& key)
{
    return m.find(key) != m.end();
}

Then use it wherever you want:

if (exists_in_unordered_map(someMap, x))
{
    // found x, now do something here
}

Answer 3

Most common practice is to use debug options like _LIBCPP_DEBUG of clang, _GLIBCXX_DEBUG of gcc or _ITERATOR_DEBUG_LEVEL of msvc in testing builds.

These will cause lot of run-time sanity checks like that unrelated iterators are not compared with each other. The defect like you describe (and many others) will be found and fixed more rapidly as result.

Programming some run-time checks or helper logics feels worse idea. Once the programming error is repaired then the check may just waste run-time performance of end product while C++ is most commonly used exactly because of that good performance.