[PATCH] D67665: [ADT] Add 2 DenseMap::insert_or_assign overloads

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: include/llvm/ADT/DenseMap.h:206-231
+  template <typename V>
+  std::pair<iterator, bool> insert_or_assign(KeyT &&Key, V &&Val) {
+    BucketT *B;
+    bool Added = !LookupBucketFor(Key, B);
+    if (Added) {
+      B = InsertIntoBucket(B, std::move(Key), std::forward<V>(Val));
+    } else {
----------------
Is this significantly cheaper than - say, try_emplace + assignment? (the latter seems like it'd keep the implementation simpler/less risky)

  auto {Iter, Inserted} = m.try_emplace(key, value);
  if (!Inserted)
    Iter->second = value

& at that point, should it perhaps be a utility, rather than reimplementing it in every container?


================
Comment at: include/llvm/ADT/DenseMap.h:226-227
+    } else {
+      B->getSecond().~ValueT();
+      ::new (&B->getSecond()) ValueT(std::forward<V>(Val));
+    }
----------------
This doesn't look like assignment - seems like it's inconsistent with the name of the function and the functionality of the standard version of this function?


================
Comment at: unittests/ADT/DenseMapTest.cpp:195
+TEST(DenseMapCustomTest, InsertOrAssignTest) {
+  DenseMap<int, std::unique_ptr<int>> Map;
+  auto Try1 = Map.insert_or_assign(0, new int(1));
----------------
I'd probably test this with something simpler than std::unique_ptr - to make the test more targeted/clear as to what it's testing.


================
Comment at: unittests/ADT/DenseMapTest.cpp:198-199
+  EXPECT_TRUE(Try1.second);
+  auto Try2 = Map.insert_or_assign(0, new int(2));
+  EXPECT_FALSE(Try2.second);
+  EXPECT_EQ(2, *Try2.first->getSecond());
----------------
Does this leak memory?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67665/new/

https://reviews.llvm.org/D67665