-
Notifications
You must be signed in to change notification settings - Fork 117
Implements __begin()
/ __end()
utility functions for ranges in C++17 and C++20
#2493
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?
Implements __begin()
/ __end()
utility functions for ranges in C++17 and C++20
#2493
Conversation
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.
Pull Request Overview
This PR implements utility functions __begin()
and __end()
for handling ranges in both C++17 and C++20 environments, providing a unified interface for accessing range iterators. The implementation uses C++20's std::ranges
API when available and falls back to trait-based detection for C++17.
- Added
__begin()
and__end()
utility functions with conditional compilation for C++17/C++20 - Introduced
__iterator_t
type alias for consistent iterator type handling - Updated existing code to use the new utilities instead of direct method calls
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
include/oneapi/dpl/pstl/utils_ranges.h | Implements the core __begin() /__end() utilities and updates existing functions to use them |
include/oneapi/dpl/pstl/hetero/algorithm_ranges_impl_hetero.h | Updates iterator type extraction to use new __iterator_t alias |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
…oneapi::dpl::__ranges::__begin() / oneapi::dpl::__ranges::__end() methods
…ranges::__empty() method based on oneapi::dpl::__ranges::__begin() and oneapi::dpl::__ranges::__end() methods
…ranges::__size() method based on oneapi::dpl::__ranges::__begin() and oneapi::dpl::__ranges::__end() methods
…:__begin() in struct __subscription_impl_view_simple::operator[]
… oneapi::dpl::__ranges::__iterator_t in __pattern_min_element_impl + _BackendTag
… oneapi::dpl::__ranges::__iterator_t in __pattern_minmax_element_impl + _BackendTag
…internal::__value_t based on oneapi::dpl::__ranges::__iterator_t
…:dpl::__internal::__value_t based on oneapi::dpl::__ranges::__iterator_t" This reverts commit 56a1c26.
… / end() methods through ADL
… / end() methods for arrays
…__ranges::__begin() / oneapi::dpl::__ranges::__end() methods
…e _IteratorValueType through oneapi::dpl::__ranges::__iterator_t<_Range> in __pattern_min_element_impl + _BackendTag and __pattern_minmax_element_impl + _BackendTag methods
…terator_t as not used anymore
…ace before __begin() calls
9dbcd3c
to
d9b5da0
Compare
…ienko/avoid_begin_end_calls_for_ranges
…f namespace before __begin() calls" This reverts commit 0b445fa.
…ace before __begin() calls
…et_count() method call to __get_buffer_size() call In file included from /oneDPL/oneDPL/test/parallel_api/ranges/adjacent_find_ranges_sycl.pass.cpp:18: In file included from /oneDPL/oneDPL/include/oneapi/dpl/execution:70: In file included from /oneDPL/oneDPL/include/oneapi/dpl/pstl/algorithm_impl.h:28: In file included from /oneDPL/oneDPL/include/oneapi/dpl/pstl/execution_impl.h:22: In file included from /oneDPL/oneDPL/include/oneapi/dpl/pstl/parallel_backend.h:32: In file included from /oneDPL/oneDPL/include/oneapi/dpl/pstl/hetero/dpcpp/parallel_backend_sycl.h:34: /oneDPL/oneDPL/include/oneapi/dpl/pstl/hetero/dpcpp/../../utils_ranges.h:187:51: error: 'get_count' is deprecated: get_count() is deprecated, please use size() instead [-Werror,-Wdeprecated-declarations] 187 | __check_size(int) -> decltype(std::declval<_R&>().get_count()); | ^ /oneDPL/oneDPL/include/oneapi/dpl/pstl/hetero/dpcpp/../../utils_ranges.h:208:52: note: while substituting explicitly-specified template arguments into function template '__check_size' 208 | using __difference_t = std::make_signed_t<decltype(__check_size<_R>(0))>; | ^ /oneDPL/oneDPL/include/oneapi/dpl/pstl/glue_algorithm_ranges_impl.h:1473:98: note: in instantiation of template type alias '__difference_t' requested here 1473 | oneapi::dpl::__internal::__enable_if_execution_policy<_ExecutionPolicy, oneapi::dpl::__internal::__difference_t<_Range>> | ^ /oneDPL/oneDPL/test/parallel_api/ranges/adjacent_find_ranges_sycl.pass.cpp:46:16: note: while substituting deduced template arguments into function template 'adjacent_find' [with _ExecutionPolicy = const oneapi::dpl::execution::device_policy<> &, _Range = sycl::buffer<int> &, _BinaryPredicate = TestUtils::IsEqual<int>] 46 | res2 = adjacent_find(CLONE_TEST_POLICY_IDX(exec, 1), A, TestUtils::IsEqual<int>()); | ^ /oneDPL/oneDPL/test/parallel_api/ranges/adjacent_find_ranges_sycl.pass.cpp:61:5: note: in instantiation of function template specialization 'test_impl<oneapi::dpl::execution::device_policy<> &>' requested here 61 | test_impl(policy); | ^ /opt/intel/oneapi/compiler/2025.2/bin/compiler/../../include/sycl/buffer.hpp:497:3: note: 'get_count' has been explicitly marked deprecated here 497 | __SYCL2020_DEPRECATED("get_count() is deprecated, please use size() instead") | ^ /opt/intel/oneapi/compiler/2025.2/bin/compiler/../../include/sycl/detail/defines_elementary.hpp:62:40: note: expanded from macro '__SYCL2020_DEPRECATED' 62 | #define __SYCL2020_DEPRECATED(message) __SYCL_DEPRECATED(message) | ^ /opt/intel/oneapi/compiler/2025.2/bin/compiler/../../include/sycl/detail/defines_elementary.hpp:53:38: note: expanded from macro '__SYCL_DEPRECATED' 53 | #define __SYCL_DEPRECATED(message) [[deprecated(message)]] | ^
|
||
template <typename _R> | ||
auto | ||
__check_size(long) -> decltype(std::declval<_R&>().get_count()); |
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 have changed this line to avoid compile error like described in the message for commit ba01762
Now it's implemented as
template <typename _R>
auto
__check_size(int) -> decltype(__get_buffer_size(std::declval<_R&>()));
#if _ONEDPL_CPP20_RANGES_PRESENT | ||
template <typename _Range> | ||
auto | ||
__size(_Range&& __rng) -> decltype(std::ranges::size(__rng)) |
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.
We should declare return type here as -> decltype(std::ranges::size(__rng))
because we using this function __size()
now in
template <typename _R>
auto
__check_size(long) -> decltype(oneapi::dpl::__ranges::__size(std::declval<_R&>()));
template <typename _It> | ||
template <typename _R> | ||
auto | ||
__check_size(...) -> typename std::iterator_traits<_It>::difference_type; |
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.
Now re doing the same task in
auto
__check_size(long long) -> typename std::iterator_traits<_It>::difference_type;
__check_size(...) -> typename std::iterator_traits<_It>::difference_type; | ||
__check_size(...) | ||
{ | ||
//static_assert should always fail when this overload is chosen, so its condition must depend on |
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.
And here we have compile error if something were not compiled (__check_size
return type not evaluated).
This helps us to understand in which type we have the problem.
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 after changes
…ize() for struct MinimalisticRange
…) / size() implementations
fc1bf0b
to
5f6ec53
Compare
…nge<ForwardIterator>) function as not required
…declaration for __begin(_Range&& __rng)
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. Could you wait for the CI to pass before merging?
…libc++ 18 and older
…ix run-time errors
…or>), end(MinimalisticRange<ForwardIterator>)
6362e53
to
73d67b6
Compare
else | ||
{ | ||
return std::vector<int, typename sycl::usm_allocator<int, sycl::usm::alloc::shared>>( | ||
typename sycl::usm_allocator<int, sycl::usm::alloc::shared>(policy.queue())); | ||
} |
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.
Apparently keeping this SYCL-dependent code under the if constexpr
condition is not sufficient - it still requires having SYCL API available, which leads to test failures. Looks like either checking a macro or some SFINAE dispatch is needed.
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.
Fixed.
decltype(std::distance(__begin(std::declval<_Range>()), __end(std::declval<_Range>())))> | ||
__size(_Range&& __rng) | ||
{ | ||
return std::distance(__begin(__rng), __end(__rng)); |
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 would like to issue a note of caution here, because it might affect perfomance.
Usually a size()
method of C++ library types "Calculates the number of elements in t in constant time."
It is true for the ranges as well. I see that it is an internal function __size
, but.. in any case the semantic and complexity should be the same, I believe.
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.
Disagree. Please take a look to around code, this code will works in case when the range passed into this function doesn't support .size()
method.
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.
Mikhail says that this overload cannot guarantee that the distance is computed in O(1)
, which impacts the overall complexity guarantee of __size
- unlike std::ranges::size
, this function does not generally guarantee constant time. That's a fair point.
However, complexity of the method size()
(as well as of std::size
) might depend on a particular container - it is usually constant time, but that is not guaranteed. So technically this overload does not make it worse.
For me, the question is whether we have added this overload just in case or it is known to be necessary in certain supported scenarios.
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 added this overload to be compliant with begin()
/ end()
methods which may be found for user range trough ADL
.
For example, we are ready to use MinimalisticRange
without size()
method.
And we have a lot of code which need to know range size and has been written not under _ONEDPL_CPP20_RANGES_PRESENT
macro.
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.
@SergeyKopienko, let me ask a "naive" question. Why do we need to call "__size()" function with an unsized range?
If you want (or a user wants) to call "size" function with an unsized range, that means you might have wrong (not effective) implementation or code design.
A quote from cppreference:
"ranges::sized_range(C++20) | specifies that a range knows its size in constant time"
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.
might depend on a particular container - it is usually constant time, but that is not guaranteed
@akukanov, If we are talking about C++ standard containers... do you know which containers (excepting std::list
and std::basic_string
before C++11) have non-constant time for "size" metod? I've checked it.. Starting from C++11 there is a "constant time" complexity requirement for the all containers, which have method size
.
(https://en.cppreference.com/w/Talk%253Acpp/named_req/Container.html)
(https://en.cppreference.com/w/cpp/container/list/size.html)
(https://en.cppreference.com/w/cpp/string/basic_string/size.html)
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.
Starting from C++11 there is a "constant time" complexity requirement for the all containers, which have method size.
Thanks for the link - I did not know the standard requirement for a "Container" guarantees constant complexity for .size()
. We can also say that the containers of interest (i.e., those with random access) do not do the linear search.
But then, std::distance
also guarantees constant complexity for random access iterators. Should we additionally constrain the overload by decltype(__begin(std::declval<_Range>())
being a random access iterator?
…ix review comment
8de3893
to
0334bea
Compare
This PR implements some new utility functions for handling ranges in both C++17 and C++20 environments, providing a unified interface for accessing range iterators:
oneapi::dpl::__ranges::__begin()
oneapi::dpl::__ranges::__end()
The implementation uses C++20's
std::ranges
API when available and falls back to trait-based detection for C++17.oneapi::dpl::__ranges::__begin()
andoneapi::dpl::__ranges::__end()
utility functions with conditional compilation for C++17/C++20__iterator_t
type alias for consistent iterator type handlingAlso in this PR some functions has been refactored and simplified:
oneapi::dpl::__ranges::__size()
oneapi::dpl::__ranges::__empty()