lambda signature for_each + unordered_map

Question

#include <unordered_map>
#include <string>
#include <iostream>
#include <algorithm>
#include <utility>

int main() 
{
  std::unordered_map<string, int> hash {{"a", 1}, {"b", 2}, {"c", 3}};

  // CaseA(NO-ERROR)
  std::for_each(hash.begin(), hash.end(),
    [](const std::pair<string, int>& p) {
      std::cout << p.first << " => " << p.second << endl;
    }
  );

  // CaseB(NO-ERROR)
  std::for_each(hash.begin(), hash.end(),
    [](const std::pair<string, int> p) {
      std::cout << p.first << " => " << p.second << endl;
    }
  );

  // CaseC(NO-ERROR)
  std::for_each(hash.begin(), hash.end(),
    [](std::pair<string, int> p) {
      std::cout << p.first << " => " << p.second << endl;
    }
  );

  // CaseD(ERROR)
  std::for_each(hash.begin(), hash.end(),
    [](std::pair<string, int>& p) {
      std::cout << p.first << " => " << p.second << endl;
    }
  );

}

Q1> Why CaseD is wrong?

Q2> Is it true that CaseA is the recommended way?

Thank you

Answer 1

Your problem is that your container is full of std::pair<const string, int>. For case 1 through 3, the std::pair<const string, int> in the container can be converted to a std::pair<string, int> implicitly, and then that temporary passed to your lambda.

The recommended way in C++11 to do something non-mutating for each element of a container is:

for( auto const& p: hash ) {
  std::cout << p.first << " => " << p.second << endl;
}

which is less verbose, and doesn't violate DRY. Prefer container-based iteration instead of iterator-based iteration when it makes sense.

Between container-based std:: algorithms and auto typed lambdas, using the std:: algorithms is going to become more tempting again in a version or two of C++. Even then, unless you are abstracting over the kind of algorithm you are using your lambda on, for_each is now quite questionable, because we have a first-class language feature that does what for_each does.

Answer 2

The value_type for a std::unordered_map<K,V> is std::pair<const K,V> (note the const). You cannot bind a reference of type std::pair<K,V> to an object of type std::pair<const K,V>. You should use std::unordered_map<K,V>::value_type instead of trying to spell the name of the type directly, as that will make sure that you don't get it wrong.

In case you wonder, case C works as there is a constructor that enables the conversion of the types, so that p will be a copy of the value in the std::unordered_map.

The recommended way for a lambda that is not meant to modify the elements in the container would be:

[](const std::unordered_map<std::string,int>::value_type& p)

In the first 3 cases in the question a copy of the element is done (performance hit). From the point of view of the caller, cases B and C are the same (in a function, the top level qualifier is dropped), but from the point of view of the definition of the lambda case B will ensure that you don't attempt to modify the argument (which is itself a copy of the source)