[libcxx-commits] [PATCH] D145285: [libc++] Refactor __less
Louis Dionne via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Thu Apr 20 09:04:50 PDT 2023
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
More information about the libcxx-commits
mailing list