[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 a comment.

In D98983#2638556 <https://reviews.llvm.org/D98983#2638556>, @Quuxplusone wrote:

> 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.

@EricWF and I were talking about this yesterday, interestingly. We agree, but we also agreed this should happen //after// everything is merged so it all remains consistent. WDYT?



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


================
Comment at: libcxx/include/concepts:395
+      const remove_reference_t<_Up>&>> &&
+  __partially_ordered_with<_Tp, _Up>;
+
----------------
Quuxplusone wrote:
> 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/
An interesting (and scary) observation. I'd suggest emailing Library Evolution and Library about this.


================
Comment at: libcxx/test/std/concepts/comparison/concepts.totallyordered/totally_ordered.compile.pass.cpp:34
+  return false;
+}
+
----------------
Quuxplusone wrote:
> 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>());
> ```
This is a subsumption check, so the fact it's not being used is by design :-)
I've added a comment to explain this since I agree that wasn't clear.


================
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 >);
+
----------------
Quuxplusone wrote:
> Inconsistent spacing here.
> Also, my perennial complaint about 25 lines that should be 1 line.
Whitespace is dictated by clang-format, and I don't care to fight with it. I'll be marking other formatting comments as done from now on.


================
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> >());
+
----------------
Quuxplusone wrote:
> 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>>`.
> I don't think these tests belong here

Right, they belong in vector tests, etc.. @ldionne asked me to put them here (as I've linked in other patches), so here they are. I think a separate (set?) of patches should migrate these to their proper homes.

> 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>>`.

Done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98983