[libcxx-commits] [PATCH] D145285: [libc++] Refactor __less

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__algorithm/comp.h:32
 
-template <class _T1, class _T2 = _T1>
-struct __less
-{
-    _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_SINCE_CXX14
-    bool operator()(const _T1& __x, const _T1& __y) const {return __x < __y;}
-
-    _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_SINCE_CXX14
-    bool operator()(const _T1& __x, const _T2& __y) const {return __x < __y;}
-
-    _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_SINCE_CXX14
-    bool operator()(const _T2& __x, const _T1& __y) const {return __x < __y;}
-
-    _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_SINCE_CXX14
-    bool operator()(const _T2& __x, const _T2& __y) const {return __x < __y;}
-};
-
-template <class _T1>
-struct __less<_T1, _T1>
-{
-    _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_SINCE_CXX14
-    bool operator()(const _T1& __x, const _T1& __y) const {return __x < __y;}
-};
-
-template <class _T1>
-struct __less<const _T1, _T1>
-{
-    _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_SINCE_CXX14
-    bool operator()(const _T1& __x, const _T1& __y) const {return __x < __y;}
-};
-
-template <class _T1>
-struct __less<_T1, const _T1>
-{
-    _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_SINCE_CXX14
-    bool operator()(const _T1& __x, const _T1& __y) const {return __x < __y;}
+template <class _T1 = void, class _T2 = _T1>
+struct __less {};
----------------
philnik wrote:
> ldionne wrote:
> > So there's actually a behavior change in this patch (the removal of implicit conversions noticed by @EricWF). However, I think it is desirable -- right now I don't think we are conforming in C++20. For example, take `lower_bound`: http://eel.is/c++draft/lower.bound#1. It specifically says `Let comp be less{} and proj be identity{} for overloads with no parameters by those names.`, which implies that we should be using the transparent comparator, but we are not (before this patch).
> > 
> > I think this mandates a change everywhere along with tests. We should also confirm what's the expected behavior in prior Standards and make sure we conform in those as well.
> I'm not actually convinced this behaviour change is reliably observable. AFAICT it would only be possible to observe with an `input_iterator` that returns a proxy reference which is comparable to the referenced type. Even then, an implementation is most likely allowed to call both `operator<(value_type&, proxy)` and `operator<(value_type&, value_type&)`. I'd argue that this case is unspecified behaviour.
I am not sure whether this is actually spelled out explicitly in the standard, but it's definitely observable: https://godbolt.org/z/51WKExTxK

At the very least, I think we need to provide this guarantee as a QOI, and so we should have tests for that under `libcxx/test/libcxx` if not in `libcxx/test/std`.


================
Comment at: libcxx/src/algorithm.cpp:24
                    RandomAccessIterator,
-                   __use_branchless_sort<Comp, RandomAccessIterator>::value>(first, last, comp, depth_limit);
+                   __use_branchless_sort<ranges::less, RandomAccessIterator>::value>(first, last, {}, depth_limit);
 }
----------------
Clearer IMO


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145285