Skip to content

feature/spaceship: Clause 22: Container Iterators #1648

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

miscco
Copy link
Contributor

@miscco miscco commented Feb 15, 2021

This implements spaceship for the container iterators

@miscco miscco requested a review from a team as a code owner February 15, 2021 21:15
@StephanTLavavej StephanTLavavej added cxx20 C++20 feature spaceship C++20 operator <=> labels Feb 16, 2021
Copy link
Contributor

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

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

What about basic_string and basic_string_view? (span is also notably covered by the Standard requirement, but we certainly have <=> for its iterators with test coverage elsewhere.)

@CaseyCarter CaseyCarter self-assigned this Feb 16, 2021
@miscco
Copy link
Contributor Author

miscco commented Feb 16, 2021

basic_string_view

Urgh char_traits and string literal overloads...

@CaseyCarter
Copy link
Contributor

basic_string_view

Urgh char_traits and string literal overloads...

The iterator <=> doesn't care about these things. (And the range <=> is in another PR so not your problem.)

@miscco
Copy link
Contributor Author

miscco commented Feb 16, 2021

basic_string_view

Urgh char_traits and string literal overloads...

The iterator <=> doesn't care about these things. (And the range <=> is in another PR so not your problem.)

I just pushed a commit for that ;) I like it when you agree that others should do the hard work

Copy link
Contributor

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

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

I've made a small change to add TRANSITION, GH-1635 to the two commented-out lines that would/could/should cover that work.

`TRANSITION` the commented-out code.
@CaseyCarter CaseyCarter removed their assignment Feb 16, 2021
Copy link
Contributor

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

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

Apply warning cleanup for tests.

Cleanup unused variable warnings.
@StephanTLavavej StephanTLavavej changed the title Implement spaceship container iterators feature/spaceship: Clause 22: Container Iterators Feb 16, 2021
@StephanTLavavej
Copy link
Member

I've pushed the following changes:

  • Fix _HAS_CXX comment typo.
  • The _Deque_const_iterator spaceship shouldn't be constexpr.
  • Mark vector and vector<bool> iterator spaceships as _CONSTEXPR20_CONTAINER.
  • Fix _STL_VERIFY message for _String_view_iterator spaceship, which isn't equality.
  • Skip _Tree_const_iterator operator!= for C++20.
  • Implement spaceships for stdext::checked_array_iterator and stdext::unchecked_array_iterator.
  • Skip _Reinterpret_move_iter operator!= for C++20. (This is an internal iterator used to make unordered_meow moves more efficient.)
  • When testing queue, there's no need to test deque iterators again, as they were tested above.
  • Activate, reorder, and comment "string iterators", "string_view iterators" tests.
  • Test checked_array_iterator and unchecked_array_iterator.

@StephanTLavavej
Copy link
Member

Thanks for implementing this Secret Bonus Round of spaceships! 🥇 🛸 😹

@StephanTLavavej StephanTLavavej removed their assignment Feb 19, 2021
@miscco miscco deleted the spaceship_container_iterators branch February 25, 2021 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cxx20 C++20 feature spaceship C++20 operator <=>
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants