Page MenuHomePhabricator

[ADT] Add 2 DenseMap::insert_or_assign overloads
Needs ReviewPublic

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

Details

Summary

I plan to use one overload in an llvm-objcopy patch.

Diff Detail

Event Timeline

MaskRay created this revision.Sep 17 2019, 9:22 AM
dexonsmith added inline comments.Sep 17 2019, 9:34 AM
include/llvm/ADT/DenseMap.h
219–222

This looks accidentally recursive to me, and I don't see test coverage for it.

MaskRay updated this revision to Diff 220531.Sep 17 2019, 10:05 AM
MaskRay added a reviewer: dexonsmith.
MaskRay removed a subscriber: dexonsmith.

Fix std::pair<iterator, bool> insert_or_assign(const KeyT &Key, V &&Val)
and add tests

MaskRay marked an inline comment as done.Sep 17 2019, 10:05 AM
dblaikie added inline comments.Sep 17 2019, 1:46 PM
include/llvm/ADT/DenseMap.h
206–231

Is this significantly cheaper than - say, try_emplace + assignment? (the latter seems like it'd keep the implementation simpler/less risky)

auto {Iter, Inserted} = m.try_emplace(key, value);
if (!Inserted)
  Iter->second = value

& at that point, should it perhaps be a utility, rather than reimplementing it in every container?

226–227

This doesn't look like assignment - seems like it's inconsistent with the name of the function and the functionality of the standard version of this function?

unittests/ADT/DenseMapTest.cpp
205

I'd probably test this with something simpler than std::unique_ptr - to make the test more targeted/clear as to what it's testing.

208–209

Does this leak memory?

MaskRay updated this revision to Diff 220605.Sep 17 2019, 8:38 PM
MaskRay marked 5 inline comments as done.

Address review comments

include/llvm/ADT/DenseMap.h
206–231

Thanks! I can do:

BucketT *B;
bool Added = !LookupBucketFor(Key, B);
if (Added)
  B = InsertIntoBucket(B, std::move(Key), std::forward<V>(Val));
else
  B->getSecond() = std::forward<V>(Val);
return std::make_pair(makeIterator(B, getBucketsEnd(), *this, true), Added);

but that seems no better than:

auto Ret = try_emplace(std::move(Key), std::forward<V>(Val));
if (!Ret.second)
  Ret.first->second = std::forward<V>(Val);
return Ret;

So I'll use the simple version.

unittests/ADT/DenseMapTest.cpp
205

I agree it is less clear but it has a nice property. std::unique_ptr<int> as the value type can be used to detect memory leaks in a -DLLVM_USE_SANITIZER=Address build.

MaskRay updated this revision to Diff 220786.Sep 18 2019, 7:03 PM

Add a comment

MaskRay updated this revision to Diff 220949.Sep 19 2019, 10:27 PM

unique_ptr<int> -> NonDefaultConstructible

MaskRay updated this revision to Diff 221922.Sep 26 2019, 4:47 AM

improve test

dexonsmith resigned from this revision.Oct 6 2020, 3:50 PM