Repository: carbon-language/carbon-lang
File: common/map_test.cpp (C++)

Code before:
template <class T, class U, class V, class W>
  requires(convertible_to<V, T> && convertible_to<W, U>)
inline auto operator==(pair<T&, U&> lhs, pair<V, W> rhs) -> bool {
  return lhs.first == static_cast<T>(rhs.first) &&
         lhs.second == static_cast<U>(rhs.second);
}
}  // namespace std
#endif

namespace Carbon::Testing {
namespace {

using RawHashtable::FixedHashKeyContext;
using RawHashtable::IndexKeyContext;
using RawHashtable::MoveOnlyTestData;
using RawHashtable::TestData;
using RawHashtable::TestKeyContext;
using ::testing::Pair;
using ::testing::UnorderedElementsAreArray;

template <typename MapT, typename MatcherRangeT>
auto ExpectMapElementsAre(MapT&& m, MatcherRangeT element_matchers) -> void {
  // Now collect the elements into a container.
  using KeyT = typename std::remove_reference<MapT>::type::KeyT;
  using ValueT = typename std::remove_reference<MapT>::type::ValueT;
  std::vector<std::pair<KeyT&, ValueT&>> map_entries;
  m.ForEach([&map_entries](KeyT& k, ValueT& v) {
    map_entries.push_back({k, v});
  });

  // Use the GoogleMock unordered container matcher to validate and show errors
  // on wrong elements.
  EXPECT_THAT(map_entries, UnorderedElementsAreArray(element_matchers));
}

// Allow directly using an initializer list.
template <typename MapT, typename MatcherT>
auto ExpectMapElementsAre(MapT&& m,
                          std::initializer_list<MatcherT> element_matchers)
    -> void {
  std::vector<MatcherT> element_matchers_storage = element_matchers;
  ExpectMapElementsAre(m, element_matchers_storage);
}

template <typename ValueCB, typename RangeT, typename... RangeTs>
auto MakeKeyValues(ValueCB value_cb, RangeT&& range, RangeTs&&... ranges)
    -> auto {
  using KeyT = typename RangeT::value_type;
  using ValueT = decltype(value_cb(std::declval<KeyT>()));
  std::vector<std::pair<KeyT, ValueT>> elements;
  auto add_range = [&](RangeT&& r) {

Review comment (suggestion):
I'm a bit surprised that this compiles... I think non-reference-wrappers here have *very* surprising behavior. I'm actually a bit surprised that the vector manages to be well behaved...

Consider this example which shows a vector of `std::pair` of reference types vs. a vector of `std::reference_wrapper` -- assigning over elements of the vector of pairs doesn't actually assign to the pair at all. Instead, the pair's assignment operator forwards the assignment to the pair's members, mutating not the vector but the underlying objects...
https://cpp.compiler-explorer.com/z/67nTv8h8G

Fortunately, `std::vector` doesn't *actually* use the assignability to do any of the growth so the code as written works... But pretty surprising things can happen:
https://cpp.compiler-explorer.com/z/n59PG3oK1

At least, personally I find that code very surprising -- I don't expect removing a pair from a vector to mutate that pair...

I would probably use `std::referenc_wrapper` here as well. And probably `const`-qualify the types. Technically, you can use `std::pair<const KeyT&, const Valuet&>`, and AFAICT this will ensure only the `std::vector` operations that *don't* assume the non-const value type is assignable compile cleanly. It's somewhat frustrating how differently this works from just a raw reference... Anyways...

Code after:
template <class T, class U, class V, class W>
  requires(convertible_to<V, T> && convertible_to<W, U>)
inline auto operator==(
    pair<std::reference_wrapper<T>, std::reference_wrapper<U>> lhs,
    pair<V, W> rhs) -> bool {
  return lhs.first == static_cast<T>(rhs.first) &&
         lhs.second == static_cast<U>(rhs.second);
}
}  // namespace std
#endif

namespace Carbon::Testing {
namespace {

using RawHashtable::FixedHashKeyContext;
using RawHashtable::IndexKeyContext;
using RawHashtable::MoveOnlyTestData;
using RawHashtable::TestData;
using RawHashtable::TestKeyContext;
using ::testing::Pair;
using ::testing::UnorderedElementsAreArray;

template <typename MapT, typename MatcherRangeT>
auto ExpectMapElementsAre(MapT&& m, MatcherRangeT element_matchers) -> void {
  // Now collect the elements into a container.
  using KeyT = typename std::remove_reference<MapT>::type::KeyT;
  using ValueT = typename std::remove_reference<MapT>::type::ValueT;
  std::vector<
      std::pair<std::reference_wrapper<KeyT>, std::reference_wrapper<ValueT>>>
      map_entries;
  m.ForEach([&map_entries](KeyT& k, ValueT& v) {
    map_entries.push_back({std::ref(k), std::ref(v)});
  });

  // Use the GoogleMock unordered container matcher to validate and show errors
  // on wrong elements.
  EXPECT_THAT(map_entries, UnorderedElementsAreArray(element_matchers));
}

// Allow directly using an initializer list.
template <typename MapT, typename MatcherT>
auto ExpectMapElementsAre(MapT&& m,
                          std::initializer_list<MatcherT> element_matchers)
    -> void {
  std::vector<MatcherT> element_matchers_storage = element_matchers;
  ExpectMapElementsAre(m, element_matchers_storage);
}

template <typename ValueCB, typename RangeT, typename... RangeTs>
auto MakeKeyValues(ValueCB value_cb, RangeT&& range, RangeTs&&... ranges)
    -> auto {