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

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

Quuxplusone requested changes to this revision.
Quuxplusone added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/test/std/concepts/concepts.compare/concept.equalitycomparable/equality_comparable.compile.pass.cpp:34
 #include "compare_types.h"
+#include "test_macros.h"
 
----------------
After my suggestion below, I think this can go away again.


================
Comment at: libcxx/test/std/concepts/concepts.compare/concept.equalitycomparable/equality_comparable.compile.pass.cpp:91-110
 static_assert(std::equality_comparable<std::array<int, 10> >);
 static_assert(std::equality_comparable<std::deque<int> >);
 static_assert(std::equality_comparable<std::forward_list<int> >);
 static_assert(std::equality_comparable<std::list<int> >);
 
 #ifndef _LIBCPP_HAS_NO_THREADS
 static_assert(!std::equality_comparable<std::lock_guard<std::mutex> >);
----------------
The test for `map` shouldn't be guarded by `_LIBCPP_HAS_NO_THREADS`. The tests involving `lock_guard` and `mutex` are just plain silly. The test for `optional<int>` should be portable even to MSVC (right?).


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116884