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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D67668#1674807 <https://reviews.llvm.org/D67668#1674807>, @MaskRay wrote:

> In D67668#1674393 <https://reviews.llvm.org/D67668#1674393>, @dblaikie wrote:
>
> > Could we have this as a standalone helper function (STLExtras or the like) rather than implementing it in every container?
>
>
> We only need it in 2 unordered associative containers: DenseMap, and StringMap. Member methods have the advantage that they have better discoverability and can be easily be completed. This method is simple enough now. Placing it in STLExtras does not save much.


There's also MapVector, I guess, but fair enough - for consistency with the standard containers it's not too much.



================
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);
----------------
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.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67668