Skip to content

lexicographical_compare_three_way - #515

Merged
Stephan T. Lavavej (StephanTLavavej) merged 19 commits into
microsoft:masterfrom
AdamBucior:lexicographical-compare-3way
Mar 8, 2020
Merged

lexicographical_compare_three_way#515
Stephan T. Lavavej (StephanTLavavej) merged 19 commits into
microsoft:masterfrom
AdamBucior:lexicographical-compare-3way

Conversation

@AdamBucior

@AdamBucior Adam Bucior (AdamBucior) commented Feb 19, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

Another part of #64. Decided to implement it in because many containers will require it for <=> operator (ex. array which is getting rid of right now #482).

Depends on #513.

Checklist

Be sure you've read README.md and understand the scope of this repo.

If you're unsure about a box, leave it unchecked. A maintainer will help you.

  • Identifiers in product code changes are properly _Ugly as per
    https://eel.is/c++draft/lex.name#3.1 or there are no product code changes.
  • The STL builds successfully and all tests have passed (must be manually
    verified by an STL maintainer before automated testing is enabled on GitHub,
    leave this unchecked for initial submission).
  • These changes introduce no known ABI breaks (adding members, renaming
    members, adding virtual functions, changing whether a type is an aggregate
    or trivially copyable, etc.).
  • These changes were written from scratch using only this repository,
    the C++ Working Draft (including any cited standards), other WG21 papers
    (excluding reference implementations outside of proposed standard wording),
    and LWG issues as reference material. If they were derived from a project
    that's already listed in NOTICE.txt, that's fine, but please mention it.
    If they were derived from any other project (including Boost and libc++,
    which are not yet listed in NOTICE.txt), you must mention it here,
    so we can determine whether the license is compatible and what else needs
    to be done.

@AdamBucior
Adam Bucior (AdamBucior) requested a review from a team as a code owner February 19, 2020 17:10
miscco
Michael Schellenberger Costa (miscco) reviewed Feb 19, 2020
View reviewed changes
Comment thread stl/inc/xutility Outdated
@StephanTLavavej

Stephan T. Lavavej (StephanTLavavej) commented Feb 26, 2020

Copy link
Copy Markdown
Member

I assume that the build failure error C2065: 'strong_ordering': undeclared identifier is caused by the #513 dependency. We'll get that merged soon to unblock this PR.

CaseyCarter
Casey Carter (CaseyCarter) suggested changes Feb 27, 2020
View reviewed changes

@CaseyCarter Casey Carter (CaseyCarter) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is missing test coverage.

Comment thread stl/inc/xutility Outdated
Comment thread stl/inc/xutility Outdated
Comment thread stl/inc/xutility Outdated
@AdamBucior @CaseyCarter
Use <=> instead of _Comp() while comparing _Ans
bbc7c18 
Co-Authored-By: Casey Carter
@AdamBucior @CaseyCarter
Use <=> in comparing _Num1 with _Num2
a12eb04 
Co-Authored-By: Casey Carter
@AdamBucior @CaseyCarter
Avoid extra iterator comparisons
e6172eb 
Co-Authored-By: Casey Carter
StephanTLavavej
Stephan T. Lavavej (StephanTLavavej) requested changes Feb 28, 2020
View reviewed changes
Comment thread stl/inc/xutility Outdated
Comment thread stl/inc/xutility Outdated
Comment thread stl/inc/xutility Outdated
Comment thread stl/inc/xutility Outdated
Comment thread stl/inc/xutility Outdated
Comment thread stl/inc/xutility Outdated
Comment thread stl/inc/xutility Outdated
Comment thread stl/inc/xutility
BillyONeal
Billy O'Neal (BillyONeal) requested changes Feb 28, 2020
View reviewed changes

@BillyONeal Billy O'Neal (BillyONeal) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs tests :)

Comment thread stl/inc/xutility Outdated
@AdamBucior

Adam Bucior (AdamBucior) commented Feb 28, 2020

Copy link
Copy Markdown
Contributor Author

This change is missing test coverage.

Needs tests :)

OK, I will try to add some tests.

@AdamBucior

Adam Bucior (AdamBucior) commented Feb 29, 2020

Copy link
Copy Markdown
Contributor Author

Should I add these tests to P0768R1_spaceship_operator or make a new folder P0768R1_lexicographical_compare_three_way?

@StephanTLavavej

Stephan T. Lavavej (StephanTLavavej) commented Feb 29, 2020

Copy link
Copy Markdown
Member

Should I add these tests to P0768R1_spaceship_operator

I think that's fine - the test is small, and this is closely related.

@AdamBucior

Adam Bucior (AdamBucior) commented Mar 2, 2020

Copy link
Copy Markdown
Contributor Author

I added some tests, I hope they're ok.

@StephanTLavavej

Stephan T. Lavavej (StephanTLavavej) commented Mar 3, 2020

Copy link
Copy Markdown
Member

We merged #513 to master, so I pushed a merge here and the build is succeeding! 😸

BillyONeal
Billy O'Neal (BillyONeal) approved these changes Mar 3, 2020
View reviewed changes
Comment thread tests/std/tests/P0768R1_spaceship_operator/test.cpp Outdated
CaseyCarter
Casey Carter (CaseyCarter) suggested changes Mar 4, 2020
View reviewed changes
Comment thread tests/std/tests/P0768R1_spaceship_operator/test.cpp Outdated
Comment thread tests/std/tests/P0768R1_spaceship_operator/test.cpp
StephanTLavavej
Stephan T. Lavavej (StephanTLavavej) requested changes Mar 7, 2020
View reviewed changes
Comment thread tests/std/tests/P0768R1_spaceship_operator/test.cpp Outdated
Comment thread tests/std/tests/P0768R1_spaceship_operator/test.cpp Outdated
Comment thread tests/std/tests/P0768R1_spaceship_operator/test.cpp Outdated
@StephanTLavavej

Stephan T. Lavavej (StephanTLavavej) commented Mar 7, 2020

Copy link
Copy Markdown
Member

My comments are small so I'm just going to push changes and batch this up for Microsoft-internal testing. 😸

@StephanTLavavej
Update test_algorithm2(), fix assert macro usage.
9c04959 
Avoid brace elision when initializing std::array.

Declare each variable on a single line.

Rename variables for clarity.

Reorder assertions for consistency.

Remove duplicated assertions.

Add additional parentheses (the preprocessor
doesn't know how to match angle brackets).
@StephanTLavavej

Stephan T. Lavavej (StephanTLavavej) commented Mar 8, 2020

Copy link
Copy Markdown
Member

I've submitted a Microsoft-internal PR for this; please don't push any more changes to this branch, and we should be able to merge it very soon. :-)

@StephanTLavavej
Stephan T. Lavavej (StephanTLavavej) merged commit 26b0629 into microsoft:master Mar 8, 2020
@StephanTLavavej

Stephan T. Lavavej (StephanTLavavej) commented Mar 8, 2020

Copy link
Copy Markdown
Member

Thanks for implementing this algorithm, Adam Bucior (@AdamBucior)! Looking at #64's checklist, it appears that you may have completed WG21-P0768 except for a feature-test macro update; I'll check with Casey Carter (@CaseyCarter). 😺

@AdamBucior
Adam Bucior (AdamBucior) deleted the lexicographical-compare-3way branch March 8, 2020 21:18
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BillyONeal Billy O'Neal (BillyONeal) BillyONeal approved these changes
@StephanTLavavej Stephan T. Lavavej (StephanTLavavej) StephanTLavavej approved these changes
+2 more reviewers
@miscco Michael Schellenberger Costa (miscco) miscco left review comments
@CaseyCarter Casey Carter (CaseyCarter) CaseyCarter approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cxx20 C++20 feature spaceship C++20 operator <=>

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@AdamBucior @StephanTLavavej @CaseyCarter @BillyONeal @miscco