[libcxx-commits] [PATCH] D116884: [libcxx][test] optional's comparisons with optional are not portably constrained

[Casey Carter via Phabricator via libcxx-commits]

CaseyCarter added inline comments.


================
Comment at: libcxx/test/std/concepts/concepts.compare/concepts.totallyordered/totally_ordered.pass.cpp:115-126
+// FIXME(cjdb): Enable when operator<=> is implemented for each of these types.
+#ifndef _LIBCPP_VERSION
+static_assert(!std::totally_ordered<std::array<A, 10> >);
+static_assert(!std::totally_ordered<std::deque<A> >);
+static_assert(!std::totally_ordered<std::forward_list<A> >);
+static_assert(!std::totally_ordered<std::list<A> >);
+#endif // _LIBCPP_VERSION
----------------
Quuxplusone wrote:
> I think "commented-out static_assert" is a much better idiom for "hey we need to fix this" purposes, than "static_assert confusingly guarded under `_LIBCPP_VERSION`."
> 
> But I see what you mean about `std::totally_ordered<std::optional<A>>` — that's `true` for libc++ but not necessarily for everyone. What about e.g. `std::list<A>` — is that assertion true for everyone (according to the paper standard)? https://eel.is/c++draft/deque.syn implies to me that it is true for everyone, because `operator<=>`'s return type is something that SFINAEs away when `T` isn't totally ordered with itself, right?
> 
> I think the appropriate diff here is simply to remove old line 120.
> 
> (Alternatively, we could change old line 120 from `static_assert` to `LIBCPP_STATIC_ASSERT`, but that feels like sneaking a libc++-specific test into `test/std/`. If we actually care about the SFINAE-friendliness of `optional<T>`'s relops (in C++17 as well as C++20), then we should write a `test/libcxx/` test for that SFINAE-friendliness. IMO it does //not// belong shoehorned into a random `concepts.compare/` test.)
> I think "commented-out static_assert" is a much better idiom for "hey we need to fix this" purposes, than "static_assert confusingly guarded under `_LIBCPP_VERSION`."

The instructions said "Uncomment when `operator<=>` is implemented for each of these types", so I did =)
 
> But I see what you mean about `std::totally_ordered<std::optional<A>>` — that's `true` for libc++ but not necessarily for everyone. What about e.g. `std::list<A>` — is that assertion true for everyone (according to the paper standard)? https://eel.is/c++draft/deque.syn implies to me that it is true for everyone, because `operator<=>`'s return type is something that SFINAEs away when `T` isn't totally ordered with itself, right?

Yes, the comparison operators for containers are all generated from constrained `<=>` (which is synthesized from `<` and `==` on the elements if they don't have `<=>`) in C++20. IIUC, libc++ hasn't yet implemented all of the `<=>` for Standard Library types.

> I think the appropriate diff here is simply to remove old line 120.

Will do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116884