[PATCH] D67668: [ADT] Add StringMap::insert_or_assign

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: include/llvm/ADT/StringMap.h:397
+  template <typename V>
+  std::pair<iterator, bool> insert_or_assign(StringRef Key, V &&Val) {
+    auto Ret = try_emplace(Key, std::forward<V>(Val));
----------------
Is this API consistent with the C++ standard version? I would guess the c++ standard version takes a pair<K, V>? I'd sort of come back to "why not a free function" if this isn't going to match the standard API & make API interoperability completely transparent.


================
Comment at: unittests/ADT/StringMapTest.cpp:273-280
+  StringMap<int> t(0);
+  auto try1 = t.insert_or_assign("a", 1);
+  EXPECT_TRUE(try1.second);
+  EXPECT_EQ(1, try1.first->second);
+  auto try2 = t.insert_or_assign("a", 2);
+  EXPECT_FALSE(try2.second);
+  EXPECT_EQ(2, try2.first->second);
----------------
dblaikie wrote:
> You could use a noisy type here to test that only the appropriate operations were executed (and/or that a type that's not default-constructible is usable here, whereas it wouldn't be if you replaced the implementation with 't[x] = y' for instance)
> 
> Countable, further down in this test file, could be used to help there.
I was thinking/hoping one of the existing test types could be used and might provide more robust checking? One that demonstrates the first insert_or_assign only calls the move constructor and the second only calls the move assignment operator?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67668