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

Unwrapping output iterators in range algorithms #5015

Merged
merged 19 commits into from
Oct 17, 2024

Conversation

CaseyCarter
Copy link
Member

@CaseyCarter CaseyCarter commented Oct 14, 2024

This completes the last bits of the iterator unwrapping design for Ranges algorithms.

Detailed commit-wise description:

  • Expand _Idl_distance design for Ranges

    • Add _Distance_unbounded to represent the distance between an iterator and unreachable_sentinel.
    • _Idl_distance takes iterator/sentinel pairs in C++20 mode.
    • Add _RANGES _Idl_distance(range), equivalent to _STD _Idl_distance but for sized_range instead of sized_sentinel_for.
    • Support integer-class counts in _Get_unwrapped_n.
  • Simplify ranges::destroy_n. Now that _Get_unwrapped_n supports integer-class counts, we don't need to apply _Algorithm_int_t to _Count.

  • Update operations on _Idl_distances

    • Refactor _Idl_dist_add:
      • Use if constexpr dispatch instead of overloading.
      • _Distance_unbounded plus anything is _Distance_unbounded.
    • Implement _Idl_dist_min to compute the smaller of two _Idl_distances.
  • Rename Range algorithm output iterator parameters from _Result to _Output. This changes our convention. I think _Output is more appropriate than _Result for this purpose given that most algorithms produce more results than just the output sequence. This also makes _Result unambiguously available to refer to the return value of the algorithm which is consistent with our use of _UResult for intermediate unwrapped results.

  • Unwrap outputs in ranges::copy

  • Unwrap outputs in ranges::copy_backward

  • Unwrap outputs in ranges::copy_if

  • Unwrap outputs in ranges::copy_n

  • Unwrap outputs in ranges::merge

  • Unwrap outputs in ranges::move

  • Unwrap outputs in ranges::move_backward

  • Unwrap outputs in ranges::replace_copy

  • Unwrap outputs in ranges::replace_copy_if

  • Unwrap outputs in ranges::reverse_copy

  • Unwrap outputs in ranges::rotate_copy

  • Unwrap outputs in ranges::sample

  • Unwrap outputs in ranges::transform (The warning suppression is necessary to prevent the static analyzer diagnosing the computation of _Min_size_determinable as redundant when _Rng1 and _Rng2 are the same type.)

Fixes #893.

@CaseyCarter CaseyCarter added enhancement Something can be improved ranges C++20/23 ranges labels Oct 14, 2024
@CaseyCarter CaseyCarter requested a review from a team as a code owner October 14, 2024 04:40
@CaseyCarter CaseyCarter force-pushed the ranges-output-unwrapping branch from cd75c40 to fd21582 Compare October 14, 2024 06:27
CaseyCarter and others added 18 commits October 14, 2024 00:30
* Add `_Distance_unbounded` to represent the distance between an iterator and `unreachable_sentinel`.
* `_Idl_distance` takes iterator/sentinel pairs in C++20 mode.
* Add `_RANGES _Idl_distance(range)`, equivalent to `_STD _Idl_distance` but for `sized_range` instead of `sized_sentinel_for`.
* Support integer-class counts in `_Get_unwrapped_n`.
Now that `_Get_unwrapped_n` supports integer-class counts, we don't need to apply `_Algorithm_int_t` to `_Count`.
* Refactor `_Idl_dist_add`:
  * Use `if constexpr` dispatch instead of overloading.
  * `_Distance_unbounded` plus anything is `_Distance_unbounded`.
* Implement `_Idl_dist_min` to compute the smaller of two `_Idl_distance`s.
…_Output`

This changes our convention. I think `_Output` is more appropriate than `_Result` for this purpose given that most algorithms produce more results than just the output sequence. This also makes `_Result` unambiguously available to refer to the return value of the algorithm which is consistent with our use of `_UResult` for intermediate unwrapped results.
These tests failed because an iterator claims to be random-access without supporting subtraction, which breaks with our pre-C++20 `_Idl_distance`. C++20-and-later `_Idl_distance` isn't fooled by such shenanigans. (We only test libc++ in `/std:c++latest` mode.)
@CaseyCarter CaseyCarter force-pushed the ranges-output-unwrapping branch from fd21582 to 0b2139b Compare October 14, 2024 07:32
@StephanTLavavej StephanTLavavej added performance Must go faster and removed enhancement Something can be improved labels Oct 14, 2024
@StephanTLavavej StephanTLavavej self-assigned this Oct 14, 2024
stl/inc/xutility Outdated Show resolved Hide resolved
stl/inc/__msvc_iter_core.hpp Outdated Show resolved Hide resolved
stl/inc/algorithm Outdated Show resolved Hide resolved
stl/inc/xutility Outdated Show resolved Hide resolved
stl/inc/algorithm Outdated Show resolved Hide resolved
@StephanTLavavej
Copy link
Member

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

@StephanTLavavej StephanTLavavej merged commit f2a381b into microsoft:main Oct 17, 2024
39 checks passed
@StephanTLavavej
Copy link
Member

Thanks for this long-awaited improvement! 😻 🪄 🎉

@CaseyCarter CaseyCarter deleted the ranges-output-unwrapping branch October 17, 2024 05:55
CaseyCarter added a commit to CaseyCarter/STL that referenced this pull request Oct 18, 2024
microsoft#5015 unskipped these, they now go through the same old codepath.
CaseyCarter added a commit to CaseyCarter/STL that referenced this pull request Oct 27, 2024
…gative counts

* `ranges::copy_n` takes a difference argument that it was blindly converting to `size_t`

Drive-by: `_Copy_n_unchecked4` no longer needs to guard its `_STL_INTERNAL_STATIC_ASSERT(_Integer_like<_SizeTy>);` for `_HAS_CXX20` since microsoft#5015 defined `_Integer_like` in pre-20 modes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Must go faster ranges C++20/23 ranges
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

<algorithm>: Unwrapping output iterators in range algorithms
2 participants