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

Wed Mar 24 10:18:52 PDT 2021

Mordante added a comment.

No real blockers for me, but I'd like to see the patch pass the CI before approving.

================
Comment at: libcxx/include/concepts:217
   is_lvalue_reference_v<_Lhs> &&
-  common_reference_with<const remove_reference_t<_Lhs>&, const remove_reference_t<_Rhs>&> &&
+  common_reference_with<__const_lvalue_ref<_Lhs>, __const_lvalue_ref<_Rhs>> &&
   requires (_Lhs __lhs, _Rhs&& __rhs) {
----------------
Quuxplusone wrote:
> cjdb wrote:
> > Mordante wrote:
> > > Minor bikeshed; since the standard type traits start with a verb I would like to do the same here. This "trait" is like `make_signed_t` so I would suggest `__make_const_lvalue_ref`.
> > If we're going to add more characters, does this still benefit us over `const remove_reference_t<_Tp>&`?
> IMHO even `__const_lvalue_ref<T>` has //already// lost the benefit of brevity. I wanted `__const_ref_t<T>` to be the spelling that creates a "const T ref"; I see no point to saying `lvalue` because, what, you expected a const //rvalue// ref? 😛I thought the trailing `_t` would be noncontroversial, but actually if people prefer `__const_ref<T>` or even `_ConstRef<T>` I think that's great.
> Anyway, sounds like a huge bikeshed and I don't want to block anything either way.
I also don't want to start a big bikeshed, but I personally prefer names to be clear. But it's no blocker for me.

================
Comment at: libcxx/test/std/concepts/comparison/concepts.totallyordered/totally_ordered.compile.pass.cpp:45
+static_assert(models_totally_ordered<int>());
+static_assert(models_totally_ordered<double>());
+static_assert(models_totally_ordered<void*>());
----------------
Quuxplusone wrote:
> cjdb wrote:
> > Mordante wrote:
> > > This feels wrong, when a double has a NaN value there's no total order. I wonder whether this is a bug in the Standard.
> > It's a common misconception that floating-point numbers can't model `totally_ordered` because of NaN.
> > 
> > From [[ http://eel.is/c++draft/cmp.concept#1.1 | cmp.concept ]]:
> > 
> > > `t < u`, `t <= u`, `t > u`, `t >= u`, `u < t`, `u <= t`, `u > t`, and `u >= t` have the same domain.
> > 
> > NaN is outside this domain, which I believe is documented by floating-point specifications.
> > 
> > Andrew Sutton sums this up in his [[ https://youtu.be/ZeU6OPaGxwM?t=1980 | CppCon 2019 talk ]]. Specifically:
> > 
> > > Concepts do not hide preconditions. Syntactic operations are not total operations. Floating point types are totally ordered even though NaN exists.... because there's a precondition on the algorithms that use the operation.... Random-access iterators are totally ordered, even though the relation is really sparse.
> +1 @cjdb. A Concept like `totally_ordered` is ultimately just a way of organizing source code, like saying "does it make sense to have a `std::set` of these things." You can make a `std::set<double>` just fine. It does misbehave badly if you try to put NaN into it; but, you just have to remember not to do that.
> 
> Andrew Sutton wrote:
> > > Random-access iterators are totally ordered, even though the relation is really sparse
> 
> Now //that's// stretching the philosophy to the breaking point, I think ;) but sure, it can make sense to have a `std::set<std::deque<T>::iterator>`, as long as you're careful to put into it only iterators from a single container and never let them become invalidated.
> 
> Or come at it from the other side: Concepts describe the requirements of algorithms, i.e. what cases the algorithm needs to worry about. If the places-where-the-natural-ordering-fails also happen to be places-where-our-algorithms-don't-care-about-the-result (maybe because they're UB), then the concept remains useful in practice.
> It's a common misconception that floating-point numbers can't model `totally_ordered` because of NaN.
> 
> From [[ http://eel.is/c++draft/cmp.concept#1.1 | cmp.concept ]]:
> 
> > `t < u`, `t <= u`, `t > u`, `t >= u`, `u < t`, `u <= t`, `u > t`, and `u >= t` have the same domain.
> 
> NaN is outside this domain, which I believe is documented by floating-point specifications.

I hadn't read that part, but http://eel.is/c++draft/concepts#concept.totallyordered-1
```
Given a type T, let a, b, and c be lvalues of type const remove_reference_t<T>.
T models totally_ordered only if

Exactly one of bool(a < b), bool(a > b), or bool(a == b) is true.
```

> Andrew Sutton sums this up in his [[ https://youtu.be/ZeU6OPaGxwM?t=1980 | CppCon 2019 talk ]]. Specifically:

Thanks for the link. I haven't seen this talk yet and Andrew's talks are usually very informative.

As said I think the code is good, just wonder about the Standard.

================
Comment at: libcxx/test/std/concepts/comparison/types.h:9
 #ifndef TEST_STD_CONCEPTS_COMPARISON_EQUALITYCOMPARABLE_H
 #define TEST_STD_CONCEPTS_COMPARISON_EQUALITYCOMPARABLE_H

----------------
Why differs the include guard from the filename? Is this a copy-paste error?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98983