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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
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 {};
----------------
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.


================
Comment at: libcxx/include/__algorithm/comp.h:33
+template <class _T1 = void, class _T2 = _T1>
+struct __less {};
+
----------------
Please add a comment explaining why it's empty.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145285