-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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 P2440R1 ranges::iota, ranges::shift_left, ranges::shift_right #2580
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this 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.
#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 |
There was a problem hiding this comment.
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!
There was a problem hiding this 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.
tests/std/tests/VSO_0157762_feature_test_macros/test.compile.pass.cpp
Outdated
Show resolved
Hide resolved
if constexpr (_Is_sized) { | ||
if (_Size < 2 * _Pos_to_shift) { | ||
return {_Buf, _Move_n_helper(_First, _Size - _Pos_to_shift, _Buf)}; | ||
} | ||
} |
There was a problem hiding this comment.
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.
I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed. |
Thanks and congratulations for implementing this significant C++23 feature! 🎉 😻 🚀 This will ship in VS 2022 17.4 Preview 1. |
…ht (microsoft#2580) Co-authored-by: Casey Carter <Casey@Carter.net> Co-authored-by: Stephan T. Lavavej <stl@nuwen.net>
Fixes #2537