Page MenuHomePhabricator

[libc++] [P1614] Implement std::compare_three_way_result[_t].
ClosedPublic

Authored by Quuxplusone on Jun 2 2021, 5:42 PM.

Diff Detail

Event Timeline

rarutyun requested review of this revision.Jun 2 2021, 5:42 PM
rarutyun created this revision.
Herald added a reviewer: Restricted Project. · View Herald TranscriptJun 2 2021, 5:42 PM
Mordante requested changes to this revision.Jun 6 2021, 3:06 AM

We recently started implementing libc++ is more granular headers. Can you adopt that style for these changes too?
You an use D103734 as an example.
I see a lot of build failure. I did a quick look and I expect none of them are due to this patch, but to main not working properly. Can you rebase your patch to see whether it passes the build?

libcxx/include/compare
149

Please update the synopsis.

libcxx/test/std/language.support/cmp/cmp.result/compare_three_way_result.pass.cpp
46 ↗(On Diff #349433)

Please make these line fit in our maximum column width of 120 characters.

76 ↗(On Diff #349433)

I'd like to see some more tests with fundamental and library types.

This revision now requires changes to proceed.Jun 6 2021, 3:06 AM
Quuxplusone added inline comments.
libcxx/test/std/language.support/cmp/cmp.result/compare_three_way_result.pass.cpp
30 ↗(On Diff #349433)
friend std::strong_ordering operator<=>(const OtherOperand&, const OtherOperand&) = delete;

but also consider whether it would break anything to make this simply

friend std::strong_ordering operator<=>(OtherOperand, OtherOperand) = delete;

because empty structs are typically passed by value. (Or even omit it entirely, although I assume that would break something you're trying to test here, is that right?)

46 ↗(On Diff #349433)

Throughout the file, make these substitutions:
SameOperandsOrdering -> SameOrder
DifferentOperandsOrdering -> DifferentOrder
OperandType -> OT
do_compare_three_way_result_positive_test -> test
do_compare_three_way_result_negative_test -> negative_test
Remove redundant usages of type traits such as add_lvalue_reference.
Use the ASSERT_SAME_TYPE test helper.
So that makes this line

ASSERT_SAME_TYPE(std::compare_three_way_result_t<OT&, OT&>, SameOrder>);

which comfortably fits within our informally enforced column limit.
Please make these changes throughout.

Quuxplusone commandeered this revision.Aug 10 2021, 7:16 PM
Quuxplusone updated this revision to Diff 365646.
Quuxplusone added a reviewer: rarutyun.
Quuxplusone retitled this revision from [libc++][compare]Implement compare_three_way_result[_t] to [libc++] [P1614] Implement std::compare_three_way_result[_t]..

Commandeer and replace with the relevant piece of D107036.

Mark the test file as requiring Concepts support. (The Mac OSX buildkite runs were failing.)

Should I do something else with this review?

@rarutyun wrote:

Should I do something else with this review?

IMO no, I hope to take it from here.
Ping @Mordante @ldionne; buildkite is happy with this; are you also happy with it?

cjdb accepted this revision.Aug 11 2021, 12:24 PM

Otherwise LGTM

libcxx/include/__compare/compare_three_way_result.h
29

We can probably use that __add_lvalue_reference_t<_Tp> alias here.

Use __make_const_lvalue_ref<X> as suggested by @cjdb.

Quuxplusone marked 2 inline comments as done.Aug 11 2021, 12:46 PM
Quuxplusone added inline comments.
libcxx/include/__compare/compare_three_way_result.h
29

Good point. Done.

ldionne accepted this revision.Aug 16 2021, 7:58 AM

LGTM, but can you please add a test that std::is_same_v<compare_three_way_result<T>, compare_three_way_result<T, T>>?

Quuxplusone marked an inline comment as done.

Add a couple new tests, and poke buildkite one more time before I land this.

This revision was not accepted when it landed; it landed in state Needs Review.Aug 18 2021, 7:02 AM
This revision was automatically updated to reflect the committed changes.