[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: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) {
----------------
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>&`?


================
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*>());
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98983