<algorithm>: ranges::minmax should dereference iterators only once
#3366
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fsb4000
changed the title
<algorithm>: ranges::minmax should dereference iterators only once
strega-nil-ms
suggested changes
Jan 27, 2023
Co-authored-by: Nicole Mazzuca <[email protected]>
timsong-cpp
reviewed
Jan 28, 2023
Co-authored-by: Alex Guteniev <[email protected]> Co-authored-by: timsong-cpp <[email protected]>
StephanTLavavej
approved these changes
Jan 30, 2023
This comment was marked as resolved.
This comment was marked as resolved.
StephanTLavavej
approved these changes
Feb 1, 2023
StephanTLavavej
approved these changes
Feb 1, 2023
|
This new pattern is unusual, but I rate it only 2 squirrels out of 5. (The Standard has an example, WG21-N4928 [dcl.init.aggr]/6, where a default member initializer refers to previous members, that's at least 3 squirrels.) I probably would have commented it, but I don't think it's worth resetting testing again. It does seem to be an improvement compared to the previous version, thanks! |
Good, I found it there: libc++ also has the bug, I created: https://reviews.llvm.org/D142864 |
strega-nil
approved these changes
Feb 1, 2023
StephanTLavavej
dismissed
strega-nil-ms’s stale review
Approved by Mirror Universe Nicole
|
I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed. |
CaseyCarter
requested changes
Feb 2, 2023
Co-authored-by: Casey Carter <[email protected]>
fsb4000
requested review from
StephanTLavavej and
CaseyCarter
and removed request for
strega-nil-ms and
StephanTLavavej
|
@StephanTLavavej |
CaseyCarter
reviewed
Feb 2, 2023
CaseyCarter
approved these changes
Feb 2, 2023
As did I. |
strega-nil-ms
approved these changes
Feb 2, 2023
StephanTLavavej
approved these changes
Feb 2, 2023
|
Everyone's joining the commit party! (I pushed tiny comment changes and removed parameter names for the defaulted function.) |
|
Thanks for minimizing bugs while maximizing performance! 📉 😸 📈 |
CaseyCarter
added a commit
to CaseyCarter/STL
that referenced
this pull request
Feb 3, 2023
The fix for microsoft#2900 in microsoft#3366 did not take advantage of the fact that the minimum and maximum elements of a range are always distinct except when the range has only one element. It couldn't easily do so due to the way `ranges::minmax` and `ranges::minmax_element` share the common backend `ranges::_Minmax_element_unchecked`. This PR introduces a new backend for `ranges::minmax` (`ranges::_Minmax_fn::_Minmax_fwd_unchecked`) and makes `ranges::_Minmax_element_unchecked` the private member `ranges::_Minmax_element_fn::_Minmax_element_fwd_unchecked`. Since the two are distinct, `_Minmax_fwd_unchecked` can deal in values instead of iterators, and we can unroll the first loop iteration to detect the single-element case naturally. Since no additional branch is needed, we can enable the fix for microsoft#2900 unconditionally. This refactoring is also probably a minor throughput and perf improvement, since `minmax` no longer needs to instantiate the "old" backend's return type and the new backend is tail-called. Drive-by: Fully qualify the names of ugly functions called by `ranges::minmax` and `ranges::minmax_element`.
CaseyCarter
added a commit
to CaseyCarter/STL
that referenced
this pull request
Feb 3, 2023
The fix for microsoft#2900 in microsoft#3366 did not take advantage of the fact that the minimum and maximum elements of a range are always distinct except when the range has only one element. It couldn't easily do so due to the way `ranges::minmax` and `ranges::minmax_element` share the common backend `ranges::_Minmax_element_unchecked`. This PR introduces a new backend for `ranges::minmax` (`ranges::_Minmax_fn::_Minmax_fwd_unchecked`) and makes `ranges::_Minmax_element_unchecked` the private member `ranges::_Minmax_element_fn::_Minmax_element_fwd_unchecked`. Since the two are distinct, `_Minmax_fwd_unchecked` can deal in values instead of iterators, and we can unroll the first loop iteration to detect the single-element case naturally. Since no additional branch is needed, we can enable the fix for microsoft#2900 unconditionally. Drive-by: Fully qualify the names of ugly functions called by `ranges::minmax` and `ranges::minmax_element`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #2900
After #2958 the original repro works, but the issue still exists.
I also found that if we have only one element we also have the issue.