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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added a comment.

All of these `.compile.pass.cpp` tests should be `.pass.cpp`. It's harmless to run them, and we don't want to accidentally nerf any runtime asserts that might sneak in over the years.



================
Comment at: libcxx/include/concepts:377
+    { __t <= __u } -> __boolean_testable;
+    { __t <= __u } -> __boolean_testable;
+    { __u <  __t } -> __boolean_testable;
----------------
Please add a regression test that catches this typo (and then fix the typo).


================
Comment at: libcxx/include/concepts:395
+      const remove_reference_t<_Up>&>> &&
+  __partially_ordered_with<_Tp, _Up>;
+
----------------
This is more a standard-defect-question than a PR-question, but: `std::same_as` went out of its way to be "symmetrical" with respect to subsumption rules, so that `same_as<T,U>` expresses the same constraint as `same_as<U,T>`.
However, `equality_comparable_with<T,U>` is disjoint from `equality_comparable_with<U,T>`; and likewise `totally_ordered_with<T,U>` is disjoint from `totally_ordered_with<U,T>`.
Is this all as-it-should-be?

Relevant: https://quuxplusone.github.io/blog/2018/11/26/remember-the-ifstream/


================
Comment at: libcxx/test/std/concepts/comparison/concepts.totallyordered/totally_ordered.compile.pass.cpp:34
+  return false;
+}
+
----------------
It looks like this overload is never used. I recommend eliminating this overload.

Then, I recommend eliminating the other overload //too//, and just using
```
static_assert(std::totally_ordered<T>);
```
everywhere you are currently writing
```
static_assert(models_totally_ordered<T>());
```


================
Comment at: libcxx/test/std/concepts/comparison/concepts.totallyordered/totally_ordered.compile.pass.cpp:90
+static_assert(!std::totally_ordered<int (S::*)() const volatile&&>);
+static_assert(!std::totally_ordered < int (S::*)() const volatile&& noexcept >);
+
----------------
Inconsistent spacing here.
Also, my perennial complaint about 25 lines that should be 1 line.


================
Comment at: libcxx/test/std/concepts/comparison/concepts.totallyordered/totally_ordered.compile.pass.cpp:104
+static_assert(models_totally_ordered<std::vector<bool> >());
+static_assert(models_totally_ordered<std::vector<int> >());
+
----------------
I don't think these tests belong here; but if they do, then it would be nice to see some negative tests too, e.g. after `struct A {};` we should have `!std::totally_ordered<std::vector<A>>` and `!std::totally_ordered<std::optional<A>>`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98983