feature/spaceship: Clause 22: Containers #1046

ahanamuk · 2020-07-15T14:33:38Z

This implements #62 for Clause 22, Containers.

StephanTLavavej

Thanks for this first batch of 🛸 work! I've reviewed everything and it looks solid; although some fixes are necessary, I think they should be relatively straightforward. (The biggest unknown is how to test synth-three-way and I would be comfortable marking that as a FIXME if you want.) Then it should be ready to be merged into the feature branch.

stl/inc/stack

stl/inc/array

tests/std/tests/P0896R4_P1614R2_spaceship/test.cpp

stl/inc/array

tests/std/tests/P0896R4_P1614R2_spaceship/test.cpp

queue<T, NonSpaceshipContainer> might be instantiated for a non-three_way_comparable NonSpaceshipContainer. In that case, it shouldn't attempt to grant friendship to the spaceship operator, because that would attempt to form compare_three_way_result_t<NonSpaceshipContainer>. As far as I can tell, there's no way in the Core Language for a queue to grant friendship to only one specialization of its non-member spaceship operator while respecting the constraint. (See WG21-N4861 [temp.friend]/9.) Instead, we need to template the friendship declaration, such that queue grants friendship to all of its spaceship operators. This is safe, just verbose.

stl/inc/array

stl/inc/list

tests/std/tests/P0896R4_P1614R2_spaceship/test.cpp

We need a workaround until `std::pair` spaceship is implemented. Reorder `smaller <=> smaller_equal` for consistency with `smaller == smaller_equal` above.

This avoids anything potentially instantiating them before the partial specialization vector<bool, Alloc> has been declared.

stl/inc/vector

Co-authored-by: Casey Carter <[email protected]>

Testing `_Differing_bits == 0` might seem like it's asking the compiler to do more work (always XORing). But it allows the compiler to see that the input to `_Countr_zero` is nonzero, and that allows it to optimize away the runtime ISA check, unconditionally emitting a `tzcnt` instruction. Then, at the end of the function, we know something that the compiler doesn't realize: `_Mask` selects a differing bit (the least significant one), so `(_Left & _Mask)` and `(_Right & _Mask)` are zero/nonzero or vice versa. Therefore, we only need to inspect one side! I've verified that each of these changes improves the codegen. Co-authored-by: Casey Carter <[email protected]>

stl/inc/vector

…o other changes).

CaseyCarter

Two small suggestions.

tests/std/test.lst

stl/inc/vector

tests/std/tests/Dev11_0135139_vector_bool_comparisons/test.cpp

stl/inc/map

stl/inc/utility

StephanTLavavej

Going to merge this into the feature branch after the tests run! 🎉 😸 🛸

I've added a note to the Spaceship Project that we need to implement spaceships for the container iterators too (which are Clause 22); that shouldn't hold up this significant PR.

StephanTLavavej · 2020-07-28T09:18:42Z

Thanks for implementing the important synth-three-way machinery and all of these operators! I can hardly contain my excitement. 😎 🛸

ahanamuk requested a review from a team as a code owner July 15, 2020 14:33

mnatsuhara assigned StephanTLavavej Jul 15, 2020

StephanTLavavej added cxx20 C++20 feature spaceship C++20 operator <=> labels Jul 15, 2020

This comment has been minimized.

Sign in to view

StephanTLavavej force-pushed the feature/spaceship branch from 52c7be9 to 8fcc25f Compare July 15, 2020 23:25

StephanTLavavej force-pushed the feature/spaceship branch from e56b1ba to 81aff9b Compare July 15, 2020 23:37

StephanTLavavej changed the title ~~Feature/spaceship~~ feature/spaceship: Clause 22: Containers Jul 15, 2020

StephanTLavavej requested changes Jul 16, 2020

View reviewed changes

StephanTLavavej removed their assignment Jul 16, 2020

mnatsuhara assigned StephanTLavavej Jul 22, 2020

ahanamuk and others added 7 commits July 22, 2020 23:27

Container spaceship changes, squashed.

732d748

incorporated all comments from code review mainly changing ifdef guards

06dfcd7

undid a change to iterator

4ca3624

updated synth_three_way to a regular struct

17e650d

Restore braces lost during merging.

c2ec451

Drop <iterator> changes from this PR.

3ed745d

Adjust comment spacing.

b7e83a4

This comment has been minimized.

Sign in to view

StephanTLavavej added 4 commits July 22, 2020 23:47

Update multimap and multiset guards.

fbc0f89

Fix all spaceship guards; STL's guidance was bogus

3c9a1a0

vector spaceship needs _Synth_three_way{}.

f6c5510

StephanTLavavej force-pushed the feature/spaceship branch from 992b036 to 1dc5c43 Compare July 23, 2020 10:52

This comment has been minimized.

Sign in to view

CaseyCarter reviewed Jul 23, 2020

View reviewed changes

stl/inc/array Show resolved Hide resolved

stl/inc/list Outdated Show resolved Hide resolved

tests/std/tests/P0896R4_P1614R2_spaceship/test.cpp Outdated Show resolved Hide resolved

tests/std/tests/P0896R4_P1614R2_spaceship/test.cpp Outdated Show resolved Hide resolved

StephanTLavavej added 4 commits July 23, 2020 15:32

Use unchecked iterators to implement comparisons.

f8ef466

Verify decltype(smaller <=> larger).

dc3e324

We need a workaround until `std::pair` spaceship is implemented. Reorder `smaller <=> smaller_equal` for consistency with `smaller == smaller_equal` above.

Test varying element strengths.

b2c5086

Verify that containers use synth-three-way.

0cfe696

This comment has been minimized.

Sign in to view

StephanTLavavej added 4 commits July 24, 2020 00:49

Work around VSO-1161663.

c9699e7

Move vector swap/comparison below vector<bool>.

7538c77

This avoids anything potentially instantiating them before the partial specialization vector<bool, Alloc> has been declared.

Refactor the vector<bool> equality optimization.

8858a09

Optimize vector<bool> spaceship and less-than.

1416165

This comment has been minimized.

Sign in to view

statementreply reviewed Jul 24, 2020

View reviewed changes

stl/inc/vector Outdated Show resolved Hide resolved

StephanTLavavej and others added 3 commits July 24, 2020 19:13

Minimally upgrade vector<bool> comparison testing.

015eae3

Implement bit reversal.

cd9a610

Use _Countr_zero() for even more efficiency.

9fd5169

Co-authored-by: Casey Carter <[email protected]>

This comment has been minimized.

Sign in to view

CaseyCarter reviewed Jul 25, 2020

View reviewed changes

stl/inc/vector Show resolved Hide resolved

stl/inc/vector Outdated Show resolved Hide resolved

StephanTLavavej added 4 commits July 27, 2020 17:04

Rename vector_bool_comparisons test.

664ac1b

Test vector<bool> comparisons.

d6acf3e

Add forward declaration, move _Vbase/_VBITS, follow synopsis order (n…

2059df3

…o other changes).

Improve comments.

0355938

CaseyCarter approved these changes Jul 28, 2020

View reviewed changes

tests/std/test.lst Outdated Show resolved Hide resolved

stl/inc/vector Outdated Show resolved Hide resolved

tests/std/tests/Dev11_0135139_vector_bool_comparisons/test.cpp Show resolved Hide resolved

StephanTLavavej added 2 commits July 27, 2020 20:53

Clarify comment.

e372f73

Rename to P1614R2_spaceship.

f906d04

CaseyCarter approved these changes Jul 28, 2020

View reviewed changes

StephanTLavavej requested changes Jul 28, 2020

View reviewed changes

stl/inc/map Outdated Show resolved Hide resolved

stl/inc/utility Outdated Show resolved Hide resolved

stl/inc/utility Outdated Show resolved Hide resolved

Comment and formatting changes.

3ec3965

StephanTLavavej approved these changes Jul 28, 2020

View reviewed changes

StephanTLavavej merged commit 889b8c8 into microsoft:feature/spaceship Jul 28, 2020

StephanTLavavej removed their assignment Aug 20, 2020

feature/spaceship: Clause 22: Containers #1046

feature/spaceship: Clause 22: Containers #1046

Uh oh!

Conversation

ahanamuk commented Jul 15, 2020 • edited by StephanTLavavej Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

StephanTLavavej left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Uh oh!

This comment has been minimized.

This comment has been minimized.

Uh oh!

Uh oh!

CaseyCarter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

StephanTLavavej left a comment

Choose a reason for hiding this comment

Uh oh!

StephanTLavavej commented Jul 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ahanamuk commented Jul 15, 2020 •

edited by StephanTLavavej

Loading