Skip to content

Implement P2440R1 ranges::iota, ranges::shift_left, ranges::shift_right#2580

Merged
StephanTLavavej merged 11 commits into
microsoft:mainfrom
MitalAshok:p2440
Jun 16, 2022
Merged

Implement P2440R1 ranges::iota, ranges::shift_left, ranges::shift_right#2580
StephanTLavavej merged 11 commits into
microsoft:mainfrom
MitalAshok:p2440

Conversation

@MitalAshok
Copy link
Copy Markdown
Contributor

@MitalAshok MitalAshok commented Feb 18, 2022

Fixes #2537

@MitalAshok MitalAshok requested a review from a team as a code owner February 18, 2022 06:05
Comment thread tests/std/tests/P0769R2_shift_left_shift_right/test.cpp Outdated
Comment thread tests/std/tests/P0769R2_shift_left_shift_right/test.cpp Outdated
Comment thread stl/inc/numeric Outdated
@fsb4000

This comment was marked as resolved.

Comment thread stl/inc/forward_list
Comment thread stl/inc/yvals_core.h
Comment thread stl/inc/numeric Outdated
Comment thread stl/inc/numeric Outdated
Copy link
Copy Markdown
Contributor

@miscco miscco left a comment

Choose a reason for hiding this comment

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

Welcome to the STL and thanks for the contribution.

This PR has already a lot of things in place. That is awesome for a first contribution!

Unfortunately, we have to completely rework the algorithms themselfs to provide the guarantees from the standard.

If you have any further questions on how to do this ask away.

Comment thread stl/inc/algorithm Outdated
Comment thread stl/inc/algorithm
Comment thread stl/inc/algorithm Outdated
Comment thread stl/inc/algorithm Outdated
Comment thread stl/inc/algorithm Outdated
Comment thread stl/inc/numeric Outdated
Comment thread stl/inc/numeric Outdated
Comment thread stl/inc/numeric Outdated
Comment thread tests/std/tests/P0769R2_shift_left_shift_right/test.cpp Outdated
Comment thread tests/std/tests/P2440R1_ranges_alg_shift_left/test.cpp Outdated
@fsb4000
Copy link
Copy Markdown
Contributor

fsb4000 commented Feb 18, 2022

That is awesome for a first contribution!

@miscco MitalAshok already created another PR: #2568

So it's not a first contribution 😸

That is still awesome although!

@StephanTLavavej StephanTLavavej added cxx23 C++23 feature ranges C++20/23 ranges labels Feb 18, 2022
@StephanTLavavej StephanTLavavej self-assigned this Feb 23, 2022
Comment thread stl/inc/yvals_core.h
Comment thread stl/inc/yvals_core.h Outdated
Comment thread stl/inc/yvals_core.h Outdated
#error __cpp_lib_shift is not defined
#elif __cpp_lib_shift != 202202L
#if __cpp_lib_shift == 201806L
#error __cpp_lib_shift is 201806L when it should be 202202L
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.

No change requested (mentioning for other reviewers): I observe that this is "new" (not strictly following the existing pattern), but it's reasonable to emit a more helpful diagnostic if this ever happens. Thanks!

Comment thread stl/inc/algorithm Outdated
Comment thread stl/inc/algorithm Outdated
Comment thread stl/inc/algorithm Outdated
Comment thread tests/std/tests/P0769R2_shift_left_shift_right/test.cpp Outdated
Comment thread tests/std/tests/P0769R2_shift_left_shift_right/test.cpp Outdated
Copy link
Copy Markdown
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.

Partial review from today's Open Code Review.

Comment thread stl/inc/xutility Outdated
Comment thread stl/inc/numeric Outdated
Comment thread stl/inc/numeric Outdated
Comment thread stl/inc/algorithm Outdated
Comment thread stl/inc/algorithm Outdated
Comment thread stl/inc/algorithm Outdated
Comment thread stl/inc/algorithm Outdated
Comment thread stl/inc/algorithm Outdated
Comment thread stl/inc/algorithm Outdated
Comment thread tests/std/tests/P2440R1_ranges_alg_shift_left/test.cpp Outdated
@CaseyCarter CaseyCarter removed their assignment Jun 10, 2022
Comment thread stl/inc/algorithm Outdated
Comment thread stl/inc/algorithm
Comment thread stl/inc/yvals_core.h Outdated
Comment thread tests/std/tests/VSO_0157762_feature_test_macros/test.compile.pass.cpp Outdated
Comment thread stl/inc/numeric Outdated
Comment thread stl/inc/algorithm Outdated
Comment thread stl/inc/algorithm Outdated
Comment thread stl/inc/algorithm Outdated
Comment thread stl/inc/algorithm
@StephanTLavavej StephanTLavavej removed their assignment Jun 14, 2022
Comment thread stl/inc/algorithm
Comment thread stl/inc/algorithm
Comment thread stl/inc/algorithm
Comment on lines +5228 to +5232
if constexpr (_Is_sized) {
if (_Size < 2 * _Pos_to_shift) {
return {_Buf, _Move_n_helper(_First, _Size - _Pos_to_shift, _Buf)};
}
}
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.

No change requested: I observe that this if constexpr (_Is_sized) could be fused into the previous or the following if constexpr (_Is_sized) (and the following one comments about this _Size < 2 * _Pos_to_shift test). However, I see an argument for splitting them up this way, so that the previous and the following conditions are narrowly focused on advancing _Buf and _Lead, so this is fine as-is.

@StephanTLavavej StephanTLavavej self-assigned this Jun 15, 2022
@StephanTLavavej
Copy link
Copy Markdown
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej changed the title Implement P2440R1 ranges::iota, ranges::shift_left, ranges::shift_right (#2537) Implement P2440R1 ranges::iota, ranges::shift_left, ranges::shift_right Jun 15, 2022
@StephanTLavavej StephanTLavavej merged commit 7c56519 into microsoft:main Jun 16, 2022
@StephanTLavavej
Copy link
Copy Markdown
Member

Thanks and congratulations for implementing this significant C++23 feature! 🎉 😻 🚀

This will ship in VS 2022 17.4 Preview 1.

fsb4000 pushed a commit to fsb4000/STL that referenced this pull request Aug 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cxx23 C++23 feature ranges C++20/23 ranges

Projects

None yet

Development

Successfully merging this pull request may close these issues.

P2440R1 ranges::iota, ranges::shift_left, ranges::shift_right

7 participants