Page MenuHomePhabricator

[ADT] Add StringMap::insert_or_assign
ClosedPublic

Authored by MaskRay on Sep 17 2019, 9:56 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

MaskRay created this revision.Sep 17 2019, 9:56 AM

It turns out that I need StringMap::insert_or_assign, instead of DenseMap, for D67656.

jkorous added inline comments.
include/llvm/ADT/StringMap.h
394 ↗(On Diff #220528)

Please add some description.

Could we have this as a standalone helper function (STLExtras or the like) rather than implementing it in every container?

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.

MaskRay updated this revision to Diff 220785.Sep 18 2019, 7:00 PM
MaskRay marked an inline comment as done.

Add a comment

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.

unittests/ADT/StringMapTest.cpp
273–280 ↗(On Diff #220785)

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.

MaskRay updated this revision to Diff 220947.Sep 19 2019, 10:15 PM
MaskRay marked an inline comment as done.

You could use a noisy type here to test that only the appropriate operations were executed

Add struct NoDefaultConstructible {...}

dblaikie added inline comments.Sep 23 2019, 9:29 AM
include/llvm/ADT/StringMap.h
397 ↗(On Diff #220947)

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.

unittests/ADT/StringMapTest.cpp
273–280 ↗(On Diff #220785)

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?

MaskRay marked 2 inline comments as done.Sep 24 2019, 12:09 AM
MaskRay added inline comments.
include/llvm/ADT/StringMap.h
397 ↗(On Diff #220947)

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.

unittests/ADT/StringMapTest.cpp
273–280 ↗(On Diff #220785)

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.

dblaikie added inline comments.Sep 24 2019, 9:28 AM
include/llvm/ADT/StringMap.h
397 ↗(On Diff #220947)

Ah, cool - thanks!

unittests/ADT/StringMapTest.cpp
273–280 ↗(On Diff #220785)

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.

dblaikie accepted this revision.Sep 24 2019, 9:33 AM

Seems good with the test fleshed out more before committing - thanks for your patience!

This revision is now accepted and ready to land.Sep 24 2019, 9:33 AM
MaskRay updated this revision to Diff 221669.Sep 24 2019, 9:49 PM

Add CountCopyAndMove to flesh out the test

This revision was automatically updated to reflect the committed changes.