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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked 2 inline comments as done.
MaskRay 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));
----------------
dblaikie wrote:
> 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.
The standard version provides two overloads:

```
template <class M> pair<iterator, bool> insert_or_assign(const key_type& k, M&& obj);
template <class M> pair<iterator, bool> insert_or_assign(key_type&& k, M&& obj);
```

This is unnecessary for StringRef, which is cheap to copy (2 words). So I simply add a method that takes a StringRef instead of creating 2 overloads.


================
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:
> 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?
There is no existing trait-based test types. I can add move constructor/assignment operator to ensure no copy assignment is called if you are strong about this.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67668