This is an archive of the discontinued LLVM Phabricator instance.

[libcxx][test] optional's comparisons with optional are not portably constrained
ClosedPublic

Authored by CaseyCarter on Jan 9 2022, 1:12 AM.

Details

Summary

so use them as concept test cases only with libc++.

Diff Detail

Event Timeline

CaseyCarter requested review of this revision.Jan 9 2022, 1:12 AM
CaseyCarter created this revision.
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptJan 9 2022, 1:12 AM
Quuxplusone requested changes to this revision.Jan 9 2022, 7:01 AM
Quuxplusone added a subscriber: Quuxplusone.
Quuxplusone added inline comments.
libcxx/test/std/concepts/concepts.compare/concept.equalitycomparable/equality_comparable.compile.pass.cpp
30

After my suggestion below, I think this can go away again.

85–96

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

libcxx/test/std/concepts/concepts.compare/concepts.totallyordered/totally_ordered.pass.cpp
121–122

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

This revision now requires changes to proceed.Jan 9 2022, 7:01 AM
Quuxplusone added inline comments.Jan 9 2022, 7:02 AM
libcxx/test/std/concepts/concepts.compare/concept.equalitycomparable/equality_comparable.compile.pass.cpp
28–29

This can go away.

CaseyCarter marked 3 inline comments as done.

Address review comments.

libcxx/test/std/concepts/concepts.compare/concepts.totallyordered/totally_ordered.pass.cpp
121–122

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.

Quuxplusone accepted this revision.Feb 9 2022, 1:17 PM

LGTM now! And fairly confident the CI failure is unrelated and long-gone, although if you want to reupload to poke CI before landing, that would also be cool. :)

This revision is now accepted and ready to land.Feb 9 2022, 1:17 PM
This revision was landed with ongoing or failed builds.Feb 9 2022, 2:00 PM
This revision was automatically updated to reflect the committed changes.

LGTM now! And fairly confident the CI failure is unrelated and long-gone, although if you want to reupload to poke CI before landing, that would also be cool. :)

Nah, I live on the edge.