[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));
----------------
MaskRay wrote:
> 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.
Ah, cool - thanks!


================
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);
----------------
MaskRay wrote:
> 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.
I was thinking something like CountCtorCopyAndMove (we might even have a generalized one of these somewhere as a test utility for all of libSupport? Or certainly they've been repeated in a few tests) that distinguishes (move/copy) assignments from (move/copy) constructions. 


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67668