-
Notifications
You must be signed in to change notification settings - Fork 113
[oneDPL] Add more copying mutating parallel range algorithms #635
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
base: main
Are you sure you want to change the base?
[oneDPL] Add more copying mutating parallel range algorithms #635
Conversation
// C++20 analogue of std::ranges::reverse_copy_truncated_result | ||
namespace oneapi::dpl::ranges { | ||
template<typename I, typename O> | ||
using reverse_copy_truncated_result = std::ranges::in_in_out_result<I, I, O>; | ||
} |
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.
@akukanov and @MikeDvorskiy, could you take a look at that addition? It may make sense to add it to be consistent with other algorithms using aliases with <algorithm>_result
semantics.
Alternatively, we could just use std::ranges::in_in_out_result
.
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.
++ @rarutyun
Even though it is only an alias, I am not sure if adding it as a public name in the oneDPL namespace is a good idea, given that the same alias will be added into C++26.
I incline towards either making it exposition-only name, like the projected value type, of just using in_in_out_result
.
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.
4df673d switches to using a plain std::ranges::in_in_out_result
.
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.
@dmitriy-sobolev, @akukanov, my first reaction is it's better to keep in_in_out_result
because it's better to have one name meaning one thing (if possible). For some weird scenarios where users try using namespace oneapi::dpl;
this alias may conflict with std::
alias while the refer to the same type (check this out).
Maybe we should add a comment (if not already) that in_in_out_result
is exactly what reverse_copy_truncated_result
aliases since C++26, so we don't see the reason to introduce our own aliases
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.
Thanks for the confirmation.
Maybe we should add a comment (if not already) that in_in_out_result is exactly what reverse_copy_truncated_result aliases since C++26
This part is done.
... so we don't see the reason to introduce our own aliases
I suppose this additional clarification should be added into the implementation.
ea935bd
to
4df673d
Compare
- ``reverse_copy`` returns ``std::ranges::in_in_out_result`` rather than its alias, | ||
``std::ranges::reverse_copy_truncated_result``. |
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.
Depending on what should be considered as the "base" C++ standard, this wording might be inaccurate. As of now, the rest of the list compares with C++20/23. The new alias for the truncated result of parallel reverse_copy
will only appear in C++26.
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.
I've updated that note in a8842c6 adding more details.
bb4ac30
to
987846a
Compare
987846a
to
a8842c6
Compare
source/elements/oneDPL/source/parallel_api/parallel_range_api.rst
Outdated
Show resolved
Hide resolved
source/elements/oneDPL/source/parallel_api/parallel_range_api.rst
Outdated
Show resolved
Hide resolved
Co-authored-by: Dan Hoeflinger <[email protected]>
Co-authored-by: Dan Hoeflinger <[email protected]>
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.
LGTM
@danhoeflinger, sorry for dismissing your approval. I've made a correction. Now, |
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.
reapproving
Adding
reverse_copy
andunique_copy
parallel range algorithms into oneDPL specification.