Skip to content

[oneDPL] Add memory parallel range algorithms #631

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

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

Conversation

dmitriy-sobolev
Copy link
Contributor

@dmitriy-sobolev dmitriy-sobolev commented Jul 20, 2025

Adding uninitialized_default_construct, uninitialized_value_construct, uninitialized_move, uninitialized_copy, uninitialized_fill and destroy parallel range algorithms into oneDPL specification.

@akukanov akukanov added the DPL label Jul 29, 2025
Comment on lines 53 to 62
// Extension of C++20 exposition-only special memory concepts as defined in [special.mem.concepts]
template<typename S, typename I>
concept no-throw-sized-sentinel-for = // exposition only
no-throw-sentinel-for<S, I> &&
std::sized_sentinel_for<S, I>;

template<typename I>
concept no-throw-bidirectional-iterator = // exposition only
no-throw-forward-iterator<I> &&
std::bidirectional_iterator<I>;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think defining 7 exposition-only concepts (yes, technically two more are needed - for sentinels and forward iterators), of which only one is used in the algorithm signatures, is an overkill.

Perhaps I would only add no-throw-sized-random-access-range as the combination of sized and random access ranges with additional semantical requirements described by words.

Tagging @rarutyun for another opinion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, based on the discussion at uxlfoundation/oneDPL@cb303af#r2109002570, it is important to specifically add the requirement for the result of dereferencing to be an lvalue reference to the iterator value type.

Copy link
Contributor Author

@dmitriy-sobolev dmitriy-sobolev Aug 1, 2025

Choose a reason for hiding this comment

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

Is not it sufficient to have std::is_lvalue_reference_v<std::iter_reference_t<I>> already requested in no-throw-input-iterator?

See:

template< class I >
concept no-throw-input-iterator =
    std::input_iterator<I> &&
    std::is_lvalue_reference_v<std::iter_reference_t<I>> &&
    std::same_as<std::remove_cvref_t<std::iter_reference_t<I>>, std::iter_value_t<I>>;

Upd. I suppose you suggest adding std::is_lvalue_reference_v<std::iter_reference_t<I>> as a part of no-throw-sized-random-access-range. Let me try...

Copy link
Contributor Author

@dmitriy-sobolev dmitriy-sobolev Aug 5, 2025

Choose a reason for hiding this comment

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

I've implemented your suggestion in b69247a. Could you check it?

What I did differently:

  1. The concept does not contain "sized" part for consistency with other algorithms.
  2. It is renamed to "nothrow" from "no-throw" to follow C++23 naming because it does not depend on the C++20 concepts now.
  3. I added is_lvalue_reference_v and same_as as I assume both are needed for "add the requirement for the result of dereferencing to be an lvalue reference to the iterator value type".
  4. I added sized_sentinel_for because it remains if I substitute each syntactic requirement (it's a part of nothrow-sentinel-for).

The list of semantic requirements inherits the properties of nothrow-sentinel-for, nothrow-bidirectional-iterator, nothrow-random-access-iterator and nothrow-random-access-range from https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2025/p3179r9.html#modify_special_mem_concepts and the base concepts from https://en.cppreference.com/w/cpp/memory/ranges/nothrow_concepts.

Copy link
Contributor

Choose a reason for hiding this comment

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

So, as I check it against the paper, with this discussion in mind, I notice that here we use: std::sized_sentinel_for<std::ranges::sentinel_t<R>, std::ranges::iterator_t<R>>
rather than
std::sized_sentinel_for<std::ranges::iterator_t<R>, std::ranges::iterator_t<R>>

which I think is what we end up with when combining the concepts in the paper.

It does makes more sense to provide the sentinel of the range to sized_sentinel_for as first argument rather than the iterator of the range, but I'm not sure if we are adding any extra requirements beyond what is proposed in the paper, so I wanted to note it.

Copy link
Contributor Author

@dmitriy-sobolev dmitriy-sobolev Aug 12, 2025

Choose a reason for hiding this comment

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

Good catch. It's a mistake.

sized_sentinel_for<I, I> is a valid requirement for an iterator. random_access_range and consequently random_access_iterator has it.

Here is a description of sized_sentinel_for' for more context:

The sized_sentinel_for concept specifies that an object of the iterator type I and an object of the sentinel type S can be subtracted to compute the distance between them in constant time.

Two random access iterators with the same type must provide the distance when subtracted from each other. So, even if std::sized_sentinel_for has sentinel in its name, we must use std::sized_sentinel_for<I, I>.

Thinking further, nothrow-random-access-iterator, which is a part of nothrow-random-access-range, has nothrow-sized-sentinel-for only to add "no-throwness". We describe that trait separately in "Semantic Requirements", so there is not need in std::sized_sentinel_for<I, I> at all - it's already included into std::random_access_iterator. I am removing this line.

@dmitriy-sobolev dmitriy-sobolev force-pushed the add-range-memory-algorithms branch from 0dc211c to b69247a Compare August 5, 2025 21:36
@danhoeflinger danhoeflinger self-requested a review August 6, 2025 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants