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

[oneDPL][ranges] A fix for ranges::transform algo - it requires all input and output ranges to be sized. #2096

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

MikeDvorskiy
Copy link
Contributor

A fix according to p3179r4.
It requires all input and output ranges to be sized for ranges::transform algo.

@MikeDvorskiy MikeDvorskiy force-pushed the dev/mdvorski/transform_sized_r branch 2 times, most recently from ef4027d to 7498faf Compare March 4, 2025 13:59
@MikeDvorskiy MikeDvorskiy requested a review from kboyarinov March 4, 2025 14:04
@MikeDvorskiy MikeDvorskiy force-pushed the dev/mdvorski/transform_sized_r branch 7 times, most recently from 6bd798a to 32189b5 Compare March 4, 2025 17:38
@MikeDvorskiy MikeDvorskiy force-pushed the dev/mdvorski/transform_sized_r branch from 32189b5 to b9607c9 Compare March 5, 2025 10:48
Copy link
Contributor

@dmitriy-sobolev dmitriy-sobolev left a comment

Choose a reason for hiding this comment

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

Looks good to me leaving aside a suggestion for std_ranges_transform.pass.

Let me leave a link to the paper and a relevant section for other reviewers and my future self: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2024/p3179r4.html#require_bounded_ranges

In the case of two input ranges or sequences, for a few algorithms - namely, mismatch, equal, and transform - it could be sufficient for just one range to be bounded and the other assumed to have at least as many elements as the bounded one.
...
However, SG9 decided to require sized_range for all inputs, with the plan to relax these constraints for transform once there is a way to statically detect infinite ranges like views::repeat (as opposed to finite unsized ranges, such as null terminated strings).

@MikeDvorskiy MikeDvorskiy force-pushed the dev/mdvorski/transform_sized_r branch from c5965f7 to 44c17de Compare March 7, 2025 14:35
Copy link
Contributor

@dmitriy-sobolev dmitriy-sobolev left a comment

Choose a reason for hiding this comment

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

LGTM.

This PR changes public API, though.
I would vote for merging it as is, because:

  • It makes the algorithm compliant with the recent developments in C++ standard.
  • This algorithm is new (it's been in oneDPL for 2 releases), which lowers the risk.

However, lets wait for the opinion of others, as we discussed separately.

@danhoeflinger
Copy link
Contributor

It seems like we also need a fix for the oneDPL specification as well:

https://github.com/uxlfoundation/oneAPI-spec/blob/71f6bf676ca0b5d258373d499403f8738298e648/source/elements/oneDPL/source/parallel_api/parallel_range_api.rst?plain=1#L314

As for how to deal with any deprecation, or removal since it has already been released both in the spec and the code, it warrants some discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants