[libcxx-commits] [PATCH] D98983: [libcxx] adds concepts `std::totally_ordered` and `std::totally_ordered_with`

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added inline comments.


================
Comment at: libcxx/include/concepts:385
+template<class _Tp>
+concept totally_ordered = equality_comparable<_Tp> && __partially_ordered_with<_Tp, _Tp>;
+
----------------
curdeius wrote:
> Using `__partially_ordered_with` here means that the same expression validity is checked twice (e.g. `t < u` and `u < t`, but here the type of t and u is the same, _Tp).
> It changes nothing for the correctness (I hope), but may be slightly less efficient.
> Would it make sense to split `__partially_ordered_with` into a "one-way" concept, something along these lines (the name is bad):
> ```
> template<class _Tp, class _Up>
> concept __oneway_ordered_with =p
>   requires(const remove_reference_t<_Tp>& __t, const remove_reference_t<_Up>& __u) {
>     { __t <  __u } -> __boolean_testable;
>     { __t >  __u } -> __boolean_testable;
>     { __t <= __u } -> __boolean_testable;
>     { __t >= __u } -> __boolean_testable;
> };
> 
> template<class _Tp, class _Up>
> concept __partially_ordered_with = __oneway_ordered_with<__Tp> && __oneway_ordered_with<__Up>;
> 
> template<class _Tp>
> concept totally_ordered = equality_comparable<_Tp> && __oneway_ordered_with<_Tp, _Tp>;
> ```
I'd prefer to stay true to spec for now, and experiment on compile-time impact if the need arises.


================
Comment at: libcxx/include/concepts:393
+    common_reference_t<
+      const remove_reference_t<_Tp>&,
+      const remove_reference_t<_Up>&>> &&
----------------
Quuxplusone wrote:
> curdeius wrote:
> > Would it make sense to add a helper for `const remove_reference_t<_Tp>&` as it starts repeating a lot?
> I'm ambivalent on all these refactoring ideas, but I'll at least offer a bikeshed name on this one. ;)
> 
> ```
> template<class _Tp>
> using __const_ref_t = const remove_reference_t<_Tp>&;
> 
>     ... totally_ordered<common_reference_t<__const_ref_t<_Tp>, __const_ref_t<_Up>>> ...
> ```
Aesthetically, I don't like it, but I think it's a good way to prevent mistakes, so done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98983