[libcxx-commits] [PATCH] D109011: [libc++] [P0919] Some belated review on D87171

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone marked 2 inline comments as done.
Quuxplusone added inline comments.


================
Comment at: libcxx/include/map:548
     _LIBCPP_INLINE_VISIBILITY
-    typename enable_if<__is_transparent<_Compare, _K2>::value, bool>::type
-    operator () ( const _K2& __x, const _CP& __y ) const
----------------
rarutyun wrote:
> ldionne wrote:
> > I think those `enable_if`s 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?
> 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
`__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.


================
Comment at: libcxx/test/std/containers/unord/unord.map/equal_range.transparent.pass.cpp:19
 // UNSUPPORTED: c++03, c++11, c++14, c++17
 
 #include <unordered_map>
----------------
ldionne wrote:
> 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?
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.


================
Comment at: libcxx/test/std/containers/unord/unord.map/equal_range_const.transparent.pass.cpp:1
-//===----------------------------------------------------------------------===//
-//
----------------
ldionne wrote:
> I'm a bit confused - where did these tests go?
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}});
```


================
Comment at: libcxx/test/support/test_transparent_unordered.h:112
   assert(c.find(SearchedType<int>(1, &conversions)) != c.end());
-  assert(conversions > 0);
-  conversions = 0;
+  assert(conversions == 1);
   assert(c.find(SearchedType<int>(2, &conversions)) != c.end());
----------------
@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.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109011