Fix incorrect code in parallel_node_hash_set emplace. (#293)

* Fix incorrect code in `parallel_node_hash_set` emplace.

* Add test for issue #292.

Author: greg7mdp

parallel_hashmap/phmap.h

@@ -3159,8 +3159,8 @@ class parallel_hash_set
         Inner& inner    = sets_[subidx(hashval)];
         auto&  set      = inner.set_;
         UniqueLock m(inner);
-        typename EmbeddedSet::template InsertSlotWithHash<true> f { inner, std::move(*slot), hashval };
-        return make_rv(PolicyTraits::apply(f, elem));
+        typename EmbeddedSet::template InsertSlotWithHash<true> f { inner.set_, std::move(*slot), hashval };
+        return make_rv(&inner, PolicyTraits::apply(std::move(f), elem));
     }
 
     template <class... Args>
@@ -3227,15 +3227,15 @@ class parallel_hash_set
     std::pair<iterator, bool> emplace(Args&&... args) {
         typename phmap::aligned_storage<sizeof(slot_type), alignof(slot_type)>::type raw;
         slot_type* slot = reinterpret_cast<slot_type*>(&raw);
-        size_t hashval  = this->hash(PolicyTraits::key(slot));
-
         PolicyTraits::construct(&alloc_ref(), slot, std::forward<Args>(args)...);
+
         const auto& elem = PolicyTraits::element(slot);
+        size_t hashval   = this->hash(PolicyTraits::key(slot));
         Inner& inner     = sets_[subidx(hashval)];
         auto&  set       = inner.set_;
         UniqueLock m(inner);
-        typename EmbeddedSet::template InsertSlotWithHash<true> f { inner, std::move(*slot), hashval };
-        return make_rv(PolicyTraits::apply(f, elem));
+        typename EmbeddedSet::template InsertSlotWithHash<true> f { inner.set_, std::move(*slot), hashval };
+        return make_rv(&inner, PolicyTraits::apply(std::move(f), elem));
     }
 
     template <class... Args>

parallel_hashmap/phmap_base.h

@@ -424,18 +424,30 @@ namespace priv {
 template <class Policy, class = void>
 struct hash_policy_traits 
 {
+   // The type of the keys stored in the hashtable.
+   using key_type = typename Policy::key_type;
 private:
-    struct ReturnKey 
-    {
-        // We return `Key` here.
-        // When Key=T&, we forward the lvalue reference.
-        // When Key=T, we return by value to avoid a dangling reference.
-        // eg, for string_hash_map.
-        template <class Key, class... Args>
-        Key operator()(Key&& k, const Args&...) const {
-            return std::forward<Key>(k);
-        }
-    };
+   struct ReturnKey {
+      template <class Key,
+                phmap::enable_if_t<std::is_lvalue_reference<Key>::value, int> = 0>
+      static key_type& Impl(Key&& k, int) {
+         return *const_cast<key_type*>(std::addressof(std::forward<Key>(k)));
+      }
+
+      template <class Key>
+      static Key Impl(Key&& k, char) {
+         return std::forward<Key>(k);
+      }
+
+      // When Key=T&, we forward the lvalue reference.
+      // When Key=T, we return by value to avoid a dangling reference.
+      // eg, for string_hash_map.
+      template <class Key, class... Args>
+      auto operator()(Key&& k, const Args&...) const
+         -> decltype(Impl(std::forward<Key>(k), 0)) {
+         return Impl(std::forward<Key>(k), 0);
+      }
+   };
 
     template <class P = Policy, class = void>
     struct ConstantIteratorsImpl : std::false_type {};
@@ -448,9 +460,6 @@ struct hash_policy_traits
     // The actual object stored in the hash table.
     using slot_type  = typename Policy::slot_type;
 
-    // The type of the keys stored in the hashtable.
-    using key_type   = typename Policy::key_type;
-
     // The argument type for insertions into the hashtable. This is different
     // from value_type for increased performance. See initializer_list constructor
     // and insert() member functions for more details.

tests/node_hash_set_test.cc

@@ -105,6 +105,14 @@ TEST(THIS_TEST_NAME, MergeExtractInsert) {
   EXPECT_THAT(set2, UnorderedElementsAre(Pointee(7), Pointee(23)));
 }
 
+TEST(THIS_TEST_NAME, Emplace) {
+   using Thing = std::tuple<size_t, double>;
+   phmap::THIS_HASH_SET<Thing> hs;
+   hs.emplace(Thing(0, 1.25));
+   hs.emplace(0, 1.3);
+   assert(hs.find(Thing(0, 1.3)) != hs.end());
+}
+
 }  // namespace
 }  // namespace priv
 }  // namespace phmap