This is an archive of the discontinued LLVM Phabricator instance.

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

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?


Please update the synopsis.

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


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.

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.