Page MenuHomePhabricator

[libc++] [P1614] Implement std::compare_three_way.
ClosedPublic

Authored by Quuxplusone on Sep 29 2021, 10:33 AM.

Details

Summary

std::strong_order() etc. depend on this. Let's get this shipped!

Diff Detail

Unit TestsFailed

TimeTest
510 mslibcxx CI Apple back-deployment macosx10.15 > libc++.std/utilities/function_objects/comparisons::transparent.pass.cpp
Script: -- : 'COMPILED WITH'; /Library/Developer/CommandLineTools/usr/bin/c++ /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com-1/llvm-project/libcxx-ci/libcxx/test/std/utilities/function.objects/comparisons/transparent.pass.cpp -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk -v --target=x86_64-apple-macosx10.15 -include /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com-1/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -nostdinc++ -I/Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com-1/llvm-project/libcxx-ci/build/x86_64-apple-system-backdeployment-10.15/include/c++/v1 -I/Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com-1/llvm-project/libcxx-ci/build/x86_64-apple-system-backdeployment-10.15/projects/libcxx/include/c++build -I/Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2a -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -Wno-macro-redefined -D_LIBCPP_HAS_NO_INCOMPLETE_FORMAT -Wno-macro-redefined -D_LIBCPP_HAS_NO_INCOMPLETE_RANGES -L/Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com-1/llvm-project/libcxx-ci/build/x86_64-apple-system-backdeployment-10.15/./lib -Wl,-rpath,/Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com-1/llvm-project/libcxx-ci/build/x86_64-apple-system-backdeployment-10.15/macos-roots/macOS/libc++/10.15 -L/Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8…
520 mslibcxx CI Apple back-deployment macosx10.9 > libc++.std/utilities/function_objects/comparisons::transparent.pass.cpp
Script: -- : 'COMPILED WITH'; /Library/Developer/CommandLineTools/usr/bin/c++ /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com-1/llvm-project/libcxx-ci/libcxx/test/std/utilities/function.objects/comparisons/transparent.pass.cpp -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk -v --target=x86_64-apple-macosx10.9 -include /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com-1/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -nostdinc++ -I/Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com-1/llvm-project/libcxx-ci/build/x86_64-apple-system-backdeployment-10.9/include/c++/v1 -I/Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com-1/llvm-project/libcxx-ci/build/x86_64-apple-system-backdeployment-10.9/projects/libcxx/include/c++build -I/Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2a -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -Wno-macro-redefined -D_LIBCPP_HAS_NO_INCOMPLETE_FORMAT -Wno-macro-redefined -D_LIBCPP_HAS_NO_INCOMPLETE_RANGES -L/Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com-1/llvm-project/libcxx-ci/build/x86_64-apple-system-backdeployment-10.9/./lib -Wl,-rpath,/Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com-1/llvm-project/libcxx-ci/build/x86_64-apple-system-backdeployment-10.9/macos-roots/macOS/libc++/10.9 -L/Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com…
540 mslibcxx CI MacOS x86_64 > llvm-libc++-shared-cfg-in.std/utilities/function_objects/comparisons::transparent.pass.cpp
Script: -- : 'COMPILED WITH'; /Library/Developer/CommandLineTools/usr/bin/c++ /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com-1/llvm-project/libcxx-ci/libcxx/test/std/utilities/function.objects/comparisons/transparent.pass.cpp -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX11.1.sdk --target=x86_64-apple-darwin19.6.0 -nostdinc++ -isystem /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com-1/llvm-project/libcxx-ci/build/generic-cxx20/include/c++/v1 -isystem /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com-1/llvm-project/libcxx-ci/build/generic-cxx20/include/c++/v1 -I /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2a -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -lc++experimental -nostdlib++ -L /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com-1/llvm-project/libcxx-ci/build/generic-cxx20/lib -Wl,-rpath,/Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com-1/llvm-project/libcxx-ci/build/generic-cxx20/lib -lc++ -pthread -o /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com-1/llvm-project/libcxx-ci/build/generic-cxx20/std/utilities/function.objects/comparisons/Output/transparent.pass.cpp.dir/t.tmp.exe

Event Timeline

Quuxplusone requested review of this revision.Sep 29 2021, 10:33 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptSep 29 2021, 10:33 AM
rarutyun added inline comments.Sep 29 2021, 4:19 PM
libcxx/test/std/utilities/function.objects/comparisons/compare_three_way.pass.cpp
10

Some steps in CI failed. I believe adding // UNSUPPORTED: libcpp-no-concepts should help

34

Why do we check std::compare_three_way_result_t in this test?

47

I would suggest to also test for std::partial_ordering::unordered and also add some tests for weak_ordering

50

For heterogeneous case does it make sense to put int and double in reverse order?

rarutyun added a comment.EditedSep 29 2021, 4:21 PM

Should we add test to check availability from <functional> header or it's already covered by this one libcxx/test/std/utilities/function.objects/comparisons/transparent.pass.cpp?

Mordante added inline comments.Sep 30 2021, 11:13 AM
libcxx/include/__compare/compare_three_way.h
30

Nit: s/_LIBCPP_INLINE_VISIBILITY/_LIBCPP_HIDE_FROM_ABI/

32

The Standard doesn't specify the noexcept, is this added intentionally?

libcxx/test/std/utilities/function.objects/comparisons/compare_three_way.pass.cpp
47

+1

libcxx/test/std/utilities/function.objects/comparisons/transparent.pass.cpp
60

Nit: the empty string isn't required in C++20.

Quuxplusone marked 8 inline comments as done.

Address review comments; add test for <functional>.

libcxx/include/__compare/compare_three_way.h
32

Yes, for consistency with std::less<void> etc.
https://eel.is/c++draft/function.objects#comparisons doesn't put the conditional-noexcept on those either, but libc++ does. (That is, libc++ puts conditional-noexcept only on the transparent less<void>; not on less<T>.)

libcxx/test/std/utilities/function.objects/comparisons/compare_three_way.pass.cpp
34

Just sanity-checking. NotThreeWayComparable is an example of a type where it is legal to ask for its compare_three_way_result_t, but in fact if you try to call compare_three_way on it, it doesn't compile. (But the actual relevant test is down on line 64; I should move them closer together, but I have no good idea how to.)

libcxx/test/std/utilities/function.objects/comparisons/transparent.pass.cpp
60

Yeah, but consistency with the earlier lines (including the wonky spacing).

Mordante accepted this revision as: Mordante.Oct 2 2021, 5:26 AM

LGTM!

ldionne accepted this revision.Oct 8 2021, 8:31 AM
ldionne added inline comments.
libcxx/include/functional
139
This revision is now accepted and ready to land.Oct 8 2021, 8:31 AM

LGTM but I have a nitpick fix (see comment) and please poke CI!

Quuxplusone marked an inline comment as done.

Update comment and poke buildkite.

Remove // -*- C++ -*- comment from generated file.

Quuxplusone added inline comments.Oct 8 2021, 2:48 PM
libcxx/test/std/utilities/function.objects/comparisons/transparent.pass.cpp
61

Bother. @ldionne, is there anything clever we could do here, to avoid splitting this one line out into a separate test marked UNSUPPORTED: libcpp-no-concepts? I'll split it out if I need to, but I'm hoping that there's a clever approach I'm forgetting about.
std::compare_three_way does not exist unless !defined(_LIBCPP_HAS_NO_CONCEPTS).

Split out the is_transparent test into its own file; mark it UNSUPPORTED: libcpp-no-concepts.
I'm expecting this to pass buildkite, and then land it tonight or tomorrow.