Ranged based "for" story

Consider the following, seemingly innocent (and completely made up) fragment of code:

typedef std::map<int, std::string> MyMap;
void foo(const MyMap& m)
{
    for(const std::pair<int, std::string>& i : m)
    {
        if(i.first)
        {
            printf("%s\n", i.second.c_str());
        }
    }
}

Looks simple enough, right? Just iterate over all elements of the map and print the value for non-zero keys. We use range-based for construct, use references, so do not expect any copies. Let’s just quickly make sure it all works as expected and consult Compiler Explorer. Uh, so that is probably more code than we expected and on top of that, if we look closely we can clearly see that we’re actually creating a copy of our variable every iteration! Spotting the problem is the most tricky (compiles without warnings), but once discovered, it should be fairly obvious in retrospect. We must be using a “wrong” type and forcing compiler to perform a conversion. Ironically, it’d be fine if we used const auto&, but we banned auto in range-based for blocks for a very similar reason – too easy to forget & and iterate over copies. So what’s wrong exactly? Well, we’re iterating over value_type elements and value_type of std::map is (note const Key):

typedef pair<const Key, T> value_type;

There’s a few ways to fix it: we can replace it with

    for(const std::pair<const int, std::string>& i : m)

    for(const MyMap::value_type& i : m)

Note: as mentioned, it compiles silently without a single warning, however modern static analysis tools should point it out (you are using one of these, right?). For example, clang-tidy has performance-implicit-conversion-in-loop, which detects exactly this case (and that’s how I noticed it in the first place…).