callWithExtractedKey fix for pair of lvalue ref, improvements

Summary:
callWithExtractedKey previously incorrectly translated
std::pair<X&,Y&>&& into X&&, Y&&, when reference collapsing rules call
instead for X& and Y&.  This diff fixes the problem, adds unit tests,
and also targets the 2-argument std::pair constructor when possible.
This means that callWithExtractedKey can be safely used on top of
libc++'s std::unordered_map::emplace without disabling the key-finding
optimization in that method.  (That lib's emplace is clever enough to
be able to search on the key before constructing a node, but does not
include the logic to unwrap the std::pair 3-arg piecewise constructor.)

Reviewed By: yfeldblum

Differential Revision: D21419822

fbshipit-source-id: d3c61e45d7fe7d8c6805342bbe63a8c866f4e306

folly/container/detail/Util.h

@@ -84,6 +84,31 @@ struct TemporaryEmplaceKey {
 // about heterogeneous lookup you can just pass a single-arg template
 // that extends std::false_type.
 
+template <typename Func, typename KeyType, typename Arg1, typename Arg2>
+auto callWithKeyAndPairArgs(
+    Func&& f,
+    KeyType const& key,
+    std::tuple<Arg1>&& first_args,
+    std::tuple<Arg2>&& second_args) {
+  return f(
+      key,
+      std::forward<Arg1>(std::get<0>(first_args)),
+      std::forward<Arg2>(std::get<0>(second_args)));
+}
+
+template <typename Func, typename KeyType, typename... Args1, typename... Args2>
+auto callWithKeyAndPairArgs(
+    Func&& f,
+    KeyType const& key,
+    std::tuple<Args1...>&& first_args,
+    std::tuple<Args2...>&& second_args) {
+  return f(
+      key,
+      std::piecewise_construct,
+      std::move(first_args),
+      std::move(second_args));
+}
+
 template <
     typename KeyType,
     template <typename> class UsableAsKey,
@@ -103,9 +128,9 @@ auto callWithExtractedKey(
     std::tuple<Args2...>&& second_args) {
   // we found a usable key in the args :)
   auto const& key = std::get<0>(first_args);
-  return f(
+  return callWithKeyAndPairArgs(
+      std::forward<Func>(f),
       key,
-      std::piecewise_construct,
       std::tuple<Arg1&&>(std::move(first_args)),
       std::tuple<Args2&&...>(std::move(second_args)));
 }
@@ -126,9 +151,9 @@ auto callWithExtractedKey(
   // we will need to materialize a temporary key :(
   TemporaryEmplaceKey<KeyType, Alloc> key(
       a, std::tuple<Args1&&...>(std::move(first_args)));
-  return f(
+  return callWithKeyAndPairArgs(
+      std::forward<Func>(f),
       const_cast<KeyType const&>(key.value()),
-      std::piecewise_construct,
       std::forward_as_tuple(std::move(key.value())),
       std::tuple<Args2&&...>(std::move(second_args)));
 }
@@ -187,12 +212,15 @@ template <
     typename U1,
     typename U2>
 auto callWithExtractedKey(Alloc& a, Func&& f, std::pair<U1, U2>&& p) {
+  // std::move(p.first) is wrong because if U1 is an lvalue reference the
+  // result will incorrectly be an rvalue ref.  static_cast here allows
+  // proper ref collapsing
   return callWithExtractedKey<KeyType, UsableAsKey>(
       a,
       std::forward<Func>(f),
       std::piecewise_construct,
-      std::forward_as_tuple(std::move(p.first)),
-      std::forward_as_tuple(std::move(p.second)));
+      std::forward_as_tuple(static_cast<U1&&>(p.first)),
+      std::forward_as_tuple(static_cast<U2&&>(p.second)));
 }
 
 // callWithConstructedKey is the set container analogue of

folly/container/test/UtilTest.cpp

@@ -16,6 +16,8 @@
 
 #include <folly/container/detail/Util.h>
 
+#include <glog/logging.h>
+
 #include <folly/Optional.h>
 #include <folly/Range.h>
 #include <folly/container/test/TrackingTypes.h>
@@ -116,8 +118,8 @@ void runKeyExtractCases(
         Tracked<0>::counts().dist(Counts{1, 0, 0, 0}) +
             Tracked<1>::counts().dist(Counts{1, 0, 0, 0}),
         expectedDist)
-        << name << "\n0 -> " << Tracked<0>::counts << "\n1 -> "
-        << Tracked<1>::counts;
+        << name << "\n0 -> " << Tracked<0>::counts() << "\n1 -> "
+        << Tracked<1>::counts();
   }
   {
     std::pair<Tracked<0> const, Tracked<1>> p{0, 0};
@@ -130,8 +132,8 @@ void runKeyExtractCases(
         Tracked<0>::counts().dist(Counts{1, 0, 0, 0}) +
             Tracked<1>::counts().dist(Counts{0, 1, 0, 0}),
         expectedDist)
-        << name << "\n0 -> " << Tracked<0>::counts << "\n1 -> "
-        << Tracked<1>::counts;
+        << name << "\n0 -> " << Tracked<0>::counts() << "\n1 -> "
+        << Tracked<1>::counts();
   }
   {
     std::pair<Tracked<0>, Tracked<1>> p{0, 0};
@@ -144,8 +146,8 @@ void runKeyExtractCases(
         Tracked<0>::counts().dist(Counts{1, 0, 0, 0}) +
             Tracked<1>::counts().dist(Counts{1, 0, 0, 0}),
         expectedDist)
-        << name << "\n0 -> " << Tracked<0>::counts << "\n1 -> "
-        << Tracked<1>::counts;
+        << name << "\n0 -> " << Tracked<0>::counts() << "\n1 -> "
+        << Tracked<1>::counts();
   }
   {
     std::pair<Tracked<0>, Tracked<1>> p{0, 0};
@@ -158,8 +160,8 @@ void runKeyExtractCases(
         Tracked<0>::counts().dist(Counts{0, 1, 0, 0}) +
             Tracked<1>::counts().dist(Counts{0, 1, 0, 0}),
         expectedDist)
-        << name << "\n0 -> " << Tracked<0>::counts << "\n1 -> "
-        << Tracked<1>::counts;
+        << name << "\n0 -> " << Tracked<0>::counts() << "\n1 -> "
+        << Tracked<1>::counts();
   }
   {
     std::pair<Tracked<2>, Tracked<3>> p{0, 0};
@@ -170,7 +172,7 @@ void runKeyExtractCases(
     //   key_type ops: Tracked<0>::counts
     //   mapped_type ops: Tracked<1>::counts
     //   key_src ops: Tracked<2>::counts
-    //   mapped_src ops: Tracked<3>::counts;
+    //   mapped_src ops: Tracked<3>::counts();
 
     // There are three strategies that could be optimal for particular
     // ratios of cost:
@@ -205,7 +207,7 @@ void runKeyExtractCases(
     //   key_type ops: Tracked<0>::counts
     //   mapped_type ops: Tracked<1>::counts
     //   key_src ops: Tracked<2>::counts
-    //   mapped_src ops: Tracked<3>::counts;
+    //   mapped_src ops: Tracked<3>::counts();
     EXPECT_EQ(
         Tracked<0>::counts().dist(Counts{0, 1, 0, 1}) +
             Tracked<1>::counts().dist(Counts{0, 0, 0, 1}) +
@@ -271,7 +273,7 @@ void runKeyExtractCases(
     //   key_type ops: Tracked<0>::counts
     //   mapped_type ops: Tracked<1>::counts
     //   key_src ops: Tracked<2>::counts
-    //   mapped_src ops: Tracked<3>::counts;
+    //   mapped_src ops: Tracked<3>::counts();
     EXPECT_EQ(
         Tracked<0>::counts().dist(Counts{0, 0, 1, 0}) +
             Tracked<1>::counts().dist(Counts{0, 0, 0, 0}) +
@@ -289,7 +291,7 @@ void runKeyExtractCases(
     //   key_type ops: Tracked<0>::counts
     //   mapped_type ops: Tracked<1>::counts
     //   key_src ops: Tracked<2>::counts
-    //   mapped_src ops: Tracked<3>::counts;
+    //   mapped_src ops: Tracked<3>::counts();
     EXPECT_EQ(
         Tracked<0>::counts().dist(Counts{0, 0, 0, 1}) +
             Tracked<1>::counts().dist(Counts{0, 0, 0, 0}) +
@@ -479,66 +481,87 @@ TEST(Util, callWithConstructedKey) {
 template <typename T>
 using IsStringPiece = std::is_same<T, StringPiece>;
 
+template <typename KeyType, typename Arg1, typename Arg2>
+struct ExpectArgTypes {
+  int which{0};
+  KeyType const* expectedAddr{nullptr};
+
+  template <typename K, typename... Args>
+  void operator()(K const&, Args&&... args) {
+    // avoid static_assert to ensure we don't affect SFINAE
+    EXPECT_TRUE((std::is_same<K, KeyType>::value)) << which;
+    using T = std::tuple<Args&&...>;
+    EXPECT_EQ(std::tuple_size<T>::value, 2) << which;
+    EXPECT_TRUE((std::is_same<std::tuple_element_t<0, T>, Arg1>::value))
+        << which;
+    EXPECT_TRUE((std::is_same<std::tuple_element_t<1, T>, Arg2>::value))
+        << which;
+
+    auto t = std::forward_as_tuple(std::forward<Args>(args)...);
+    EXPECT_TRUE(expectedAddr == nullptr || expectedAddr == &std::get<0>(t))
+        << which;
+  }
+};
+
 TEST(Util, callWithExtractedHeterogeneousKey) {
   std::allocator<char> a;
   std::string str{"key"};
   StringPiece sp{"key"};
   char const* ptr{"key"};
+  auto strPair = std::make_pair(str, 0);
+  std::pair<std::string&, int> strLRefPair(str, 0);
+  std::pair<std::string&&, int> strRRefPair(std::move(str), 0);
+  auto spPair = std::make_pair(sp, 0);
+  auto ptrPair = std::make_pair(ptr, 0);
+
+  // none of the std::move below actually get consumed
+
+  detail::callWithExtractedKey<std::string, IsStringPiece>(
+      a, ExpectArgTypes<std::string, std::string&, int&&>{0, &str}, str, 0);
   detail::callWithExtractedKey<std::string, IsStringPiece>(
       a,
-      [](auto const& key, auto&&... args) {
-        // avoid static_assert to ensure we don't affect SFINAE
-        EXPECT_TRUE((std::is_same<decltype(key), std::string const&>::value));
-        using T = std::tuple<decltype(args)&&...>;
-        EXPECT_EQ(std::tuple_size<T>::value, 3);
-        EXPECT_TRUE((std::is_same<
-                     std::tuple_element_t<1, T>,
-                     std::tuple<std::string&>&&>::value));
-        EXPECT_TRUE(
-            (std::is_same<std::tuple_element_t<2, T>, std::tuple<int&&>&&>::
-                 value));
-      },
-      str,
-      0);
+      ExpectArgTypes<std::string, std::string const&, int const&>{
+          1, &strPair.first},
+      strPair);
   detail::callWithExtractedKey<std::string, IsStringPiece>(
       a,
-      [](auto const& key, auto&&... args) {
-        EXPECT_TRUE((std::is_same<decltype(key), std::string const&>::value));
-        using T = std::tuple<decltype(args)&&...>;
-        EXPECT_EQ(std::tuple_size<T>::value, 3);
-        EXPECT_TRUE((std::is_same<
-                     std::tuple_element_t<1, T>,
-                     std::tuple<std::string&&>&&>::value));
-      },
+      ExpectArgTypes<std::string, std::string&, int&&>{2, &str},
+      std::move(strLRefPair));
+  detail::callWithExtractedKey<std::string, IsStringPiece>(
+      a,
+      ExpectArgTypes<std::string, std::string&, int const&>{10, &str},
+      strLRefPair);
+
+  detail::callWithExtractedKey<std::string, IsStringPiece>(
+      a,
+      ExpectArgTypes<std::string, std::string&&, int&&>{3, &str},
       std::move(str),
       0);
   detail::callWithExtractedKey<std::string, IsStringPiece>(
       a,
-      [](auto const& key, auto&&... args) {
-        EXPECT_TRUE((std::is_same<decltype(key), StringPiece const&>::value));
-        using T = std::tuple<decltype(args)&&...>;
-        EXPECT_EQ(std::tuple_size<T>::value, 3);
-        EXPECT_TRUE((std::is_same<
-                     std::tuple_element_t<1, T>,
-                     std::tuple<StringPiece&>&&>::value));
-      },
-      sp,
-      0);
+      ExpectArgTypes<std::string, std::string&&, int&&>{4, &strPair.first},
+      std::move(strPair));
   detail::callWithExtractedKey<std::string, IsStringPiece>(
       a,
-      [](auto const& key, auto&&... args) {
-        // avoid static_assert to ensure we don't affect SFINAE
-        EXPECT_TRUE((std::is_same<decltype(key), std::string const&>::value));
-        using T = std::tuple<decltype(args)&&...>;
-        EXPECT_EQ(std::tuple_size<T>::value, 3);
-        EXPECT_TRUE((std::is_same<
-                     std::tuple_element_t<1, T>,
-                     std::tuple<std::string&&>&&>::value));
-        auto t = std::forward_as_tuple(std::forward<decltype(args)>(args)...);
-        EXPECT_EQ(&key, &std::get<0>(std::get<1>(t)));
-      },
-      ptr,
-      0);
+      ExpectArgTypes<std::string, std::string&, int const&>{5, &str},
+      strRRefPair);
+  detail::callWithExtractedKey<std::string, IsStringPiece>(
+      a,
+      ExpectArgTypes<std::string, std::string&&, int&&>{11, &str},
+      std::move(strRRefPair));
+
+  detail::callWithExtractedKey<std::string, IsStringPiece>(
+      a, ExpectArgTypes<StringPiece, StringPiece&, int&&>{6, &sp}, sp, 0);
+  detail::callWithExtractedKey<std::string, IsStringPiece>(
+      a,
+      ExpectArgTypes<StringPiece, StringPiece const&, int const&>{
+          7, &spPair.first},
+      spPair);
+
+  detail::callWithExtractedKey<std::string, IsStringPiece>(
+      a, ExpectArgTypes<std::string, std::string&&, int&&>{8}, ptr, 0);
+  detail::callWithExtractedKey<std::string, IsStringPiece>(
+      a, ExpectArgTypes<std::string, std::string&&, int const&>{9}, ptrPair);
 }
 
 TEST(Util, callWithConstructedHeterogeneousKey) {