Page MenuHomePhabricator

[libc++] [P0919] Some belated review on D87171
ClosedPublic

Authored by Quuxplusone on Aug 31 2021, 11:35 AM.

Details

Reviewers
ldionne
rarutyun
Group Reviewers
Restricted Project
Commits
rGd5db71d19f11: [libc++] [P0919] Some belated review on D87171.
Summary

A comment today on D87171 made me look at it, and then I made a bunch of review comments before finally noticing that it was shipped ages ago. 😛 Well, rather than let my comments go to waste, here's the gist of them.

  • Simplify the structure of the new tests.
  • Test const containers as well as non-const containers, since it's easy to do so.
  • Remove redundant enable-iffing of helper structs' member functions. (They're not instantiated unless they're called, and who would call them?)
  • Drive-by: _LIBCPP_INLINE_VISIBILITY on some swap functions where I don't see why it was missing in the first place.

Diff Detail

Event Timeline

Quuxplusone requested review of this revision.Aug 31 2021, 11:35 AM
Quuxplusone created this revision.
Herald added a reviewer: Restricted Project. · View Herald TranscriptAug 31 2021, 11:35 AM
ldionne requested changes to this revision.Aug 31 2021, 12:06 PM
ldionne added inline comments.
libcxx/include/map
548

I think those enable_ifs are important. Otherwise, if a non-transparent comparator is used with arguments that would trigger a conversion if one of the overloads above were used, it would be possible to observe the fact that a conversion is not being performed. Am I misunderstanding?

libcxx/test/std/containers/unord/unord.map/equal_range.transparent.pass.cpp
19

Why is this test named libcxx/test/std/containers/unord/unord.map/equal_range.transparent.pass.cpp since we seem to be testing both transparent and non-transparent comparators in it?

libcxx/test/std/containers/unord/unord.map/equal_range_const.transparent.pass.cpp
1

I'm a bit confused - where did these tests go?

This revision now requires changes to proceed.Aug 31 2021, 12:06 PM
rarutyun added inline comments.Aug 31 2021, 12:30 PM
libcxx/include/map
548

I have the same understanding. We might end up with breaking change (if I am not mistaken) when Comparator is not transparent and the key_type and K are not comparable

Quuxplusone marked 2 inline comments as done.Aug 31 2021, 12:37 PM
Quuxplusone added inline comments.
libcxx/include/map
548

__map_value_compare is used only in member functions like map::find, where map::find is already SFINAEd appropriately. The user never gets their hands on an object of type __map_value_compare to call __map_value_compare::operator() directly; all the call-sites are under our control.

You did briefly scare me that maybe __map_value_compare was being aliased as map::value_compare (which is user-accessible); but no, map::value_compare is its own separate type, and __map_value_compare remains inaccessible AFAICT.

libcxx/test/std/containers/unord/unord.map/equal_range.transparent.pass.cpp
19

Well, it's not testing pre-C++20 comparators; it's just neg-testing the new C++20 behavior: i.e. that the transparent equal_range is correctly SFINAEd away when the comparator type is non-transparent. I think this (existing) organization is pretty reasonable.

libcxx/test/std/containers/unord/unord.map/equal_range_const.transparent.pass.cpp
1

The old files foo_const.transparent.pass.cpp and foo_non_const.transparent.pass.cpp have been combined into foo.transparent.pass.cpp. The only difference between the const and non-const files is now handled by e.g.

test_transparent_equal_range<M>({{1, 2}, {2, 3}});
test_transparent_equal_range<const M>({{1, 2}, {2, 3}});
libcxx/test/support/test_transparent_unordered.h
112

@ldionne: In case it helps, notice here that we know conversions must be 1, not any other number, because when find is non-transparent, it takes a parameter of type const StoredType<int>&, not const K&. So the conversion that happens, happens immediately to the argument on line 111. The innards of non-transparent find don't even know that SearchedType exists at all. (And therefore they never try to pass a SearchedType to __map_value_compare or __unordered_map_equal or anything.)

rarutyun added inline comments.Aug 31 2021, 1:21 PM
libcxx/test/std/containers/unord/unord.map/equal_range.transparent.pass.cpp
31

I don't mind to combine const and non-const overloads of API into one file. I don't recall why I decided to make them separate. I believe there was some precedence in tests already and I made it for the sake of consistency but I cannot tell for sure.

Quuxplusone marked an inline comment as done.Sep 9 2021, 6:22 PM

@ldionne ping! (I believe my replies adequately address your comments without any new code updates needed.)

Fix some more indentation, and use a different method of SFINAE (to make it easier to match the surrounding indentation to the pre-existing practice) in <unordered_map>. @ldionne ping!

ldionne accepted this revision.Sep 20 2021, 10:47 AM

Indeed, I think all my concerns have been resolved now. Thanks.

This revision is now accepted and ready to land.Sep 20 2021, 10:47 AM