Skip to content
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

Implement ranges::rotate and ranges::rotate_copy #1073

Merged
merged 7 commits into from
Jul 30, 2020

Conversation

miscco
Copy link
Contributor

@miscco miscco commented Jul 21, 2020

I have been rotating to implement these algorithms but ended up with an ICE in my hand.

@miscco miscco requested a review from a team as a code owner July 21, 2020 20:49
@CaseyCarter CaseyCarter added cxx20 C++20 feature ranges C++20/23 ranges labels Jul 21, 2020
@CaseyCarter CaseyCarter mentioned this pull request Jul 22, 2020
stl/inc/algorithm Show resolved Hide resolved
stl/inc/algorithm Outdated Show resolved Hide resolved
stl/inc/algorithm Outdated Show resolved Hide resolved
stl/inc/algorithm Outdated Show resolved Hide resolved
stl/inc/algorithm Show resolved Hide resolved
stl/inc/algorithm Outdated Show resolved Hide resolved
stl/inc/algorithm Outdated Show resolved Hide resolved
stl/inc/algorithm Outdated Show resolved Hide resolved
tests/std/tests/P0896R4_ranges_alg_rotate_copy/test.cpp Outdated Show resolved Hide resolved
@miscco
Copy link
Contributor Author

miscco commented Jul 28, 2020

Urgh, from all the algorithms where I would not want to reduce an ICE out from rotate is high in the list

@CaseyCarter
Copy link
Member

Urgh, from all the algorithms where I would not want to reduce an ICE out from rotate is high in the list

I've got this fixed, just typing up a commit message.

* Unqualify calls to `_Reverse_common`, which cannot be confused with a `_Meow_unchecked` function in `_STD`.
* In `_Rotate_unchecked`:
  * Name the iterator value that's equal to the sentinel `_Final` instead of `_End` which has become conventional.
  * Avoid making an extra traversal ahead-of-time to find the final iterator of non-bidi ranges.
  * When we do need to find the final iterator value, start any traversal at `_Mid` instead of `_First`.
  * Let's not overload "sentinel" - which has enough meanings in Ranges - for `_Reverse_until_sentinel_unchecked`.
* Don't test `constexpr rotate` at all with MSVC: it's extremely sensitive to VSO-938163.
* Remove unnecessary bug workarounds from the `rotate_copy` test.
@CaseyCarter
Copy link
Member

I've got this fixed, just typing up a commit message.

Correction: I had this fixed, and somehow managed to break it again while typing a commit message.

@CaseyCarter CaseyCarter self-requested a review July 28, 2020 17:01
@CaseyCarter CaseyCarter removed their assignment Jul 28, 2020
@CaseyCarter

This comment has been minimized.

@azure-pipelines

This comment has been minimized.

@StephanTLavavej StephanTLavavej changed the title Implement ranges::rotate Implement ranges::rotate and ranges::rotate_copy Jul 29, 2020
Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

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

good to me! Looks

@CaseyCarter CaseyCarter self-assigned this Jul 29, 2020
@CaseyCarter CaseyCarter merged commit 58a8a39 into microsoft:master Jul 30, 2020
@CaseyCarter
Copy link
Member

"That's obviously a rotate." -- Sean Parent

@CaseyCarter CaseyCarter removed their assignment Jul 30, 2020
@miscco miscco deleted the ranges_rotate branch August 2, 2020 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cxx20 C++20 feature ranges C++20/23 ranges
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants