This is an archive of the discontinued LLVM Phabricator instance.

Part 1 of N4279 - "Improved insertion interface for unique-key maps"
ClosedPublic

Authored by mclow.lists on Jun 23 2015, 1:49 PM.

Details

Reviewers
EricWF
Summary

N4279 added try_emplace and insert_or_assign to std::map and std::unordered_map.
This patch implements the changes to map.

Diff Detail

Event Timeline

mclow.lists retitled this revision from to Part 1 of N4279 - "Improved insertion interface for unique-key maps".
mclow.lists updated this object.
mclow.lists edited the test plan for this revision. (Show Details)
mclow.lists added a reviewer: EricWF.
mclow.lists added a subscriber: Unknown Object (MLST).

Changed a couple of moves to forwards, and made the tests better.
I have an analogous set of changes to <unordered_map>, but since they're almost completely the same, I figured that they would add noise w/o value to this,

Updated the diff with the changes from LWG#2464

mclow.lists added inline comments.Jun 26 2015, 1:32 PM
include/map
1138

I think this should be __h, not __p, since __p == end()

1151

Same as #1138

EricWF edited edge metadata.Jun 29 2015, 1:45 PM

A couple of inline comments. More to come.

include/map
1098

What kind of terrible compiler provides an C++11-incomplete C++17 implementation? It would be nice if we could move away from using these macros in anything C++14 and beyond.

However I don't object if you think they are needed.

test/std/containers/associative/map/map.modifiers/insert_or_assign.pass.cpp
63

Same comment as above. Should we hide these C++17 failures for non-conforming compilers?

EricWF accepted this revision.Jul 6 2015, 1:47 PM
EricWF edited edge metadata.

LGTM modulo inline comments.

include/map
1138

I actually think you should leave this as is. __tree seems to properly handle the end iterator as a hint. __tree::__find_equal(hint, ...) assumes that if hint is end() then the value should be inserted just prior to hint.

You should be able to implement the the try_emplace(hint, ...) methods in terms of the normal try_emplace methods because we ignore the hint anyway.

test/std/containers/associative/map/map.modifiers/try.emplace.pass.cpp
70

Why not use try_emplace here as well?

73

For the sake of covering corner cases could we test every inserted value? This applies for insert_or_assign as well.

for (int i=0; i < 20; i += 2) {
  Movable mv1(i, (double)i);
  r = m.try_emplace(2, std::move(mv1));
  assert(m.size() == 10);
  assert(!r.second);                  // was not inserted
  assert(!mv1.moved());               // was not moved from
  assert(r.first->first == 2);        // key
}
80

We should test the cases where insertion happens at the front and at the back.

This revision is now accepted and ready to land.Jul 6 2015, 1:47 PM
mclow.lists added inline comments.Jul 6 2015, 8:30 PM
test/std/containers/associative/map/map.modifiers/try.emplace.pass.cpp
73

This doesn't really buy us anything, because it won't look at the value (mv1) unless the key doesn't exist. So this code is not testing every inserted value.

I think that this is what you meant:

for (int i=0; i < 20; i += 2) {
  Movable mv1(i, (double)i);
  r = m.try_emplace(i, std::move(mv1));
  assert(m.size() == 10);
  assert(!r.second);                  // was not inserted
  assert(!mv1.moved());               // was not moved from
  assert(r.first->first == i);        // key
}
80

If you want; though the behavior that we're checking for here is "was this inserted?" and "was the value moved from?", which are dependent upon "Did the value exist in the map before", as opposed to the position in the map.

mclow.lists closed this revision.Jul 6 2015, 8:39 PM

Committed as r241539.