Add explicit deduction guides for blocked_nd_range #1525
Conversation
kboyarinov
force-pushed
the
rarutyun/blocked_range_nd_class
branch
from
3fe65dc to
64610c6
Compare
… into dev/kboyarinov/blocked_range_nd_deduction_guides_test
kboyarinov
changed the title
kboyarinov
requested review from
vossmjp,
akukanov,
pavelkumbrasev and
rarutyun
… into dev/kboyarinov/blocked_range_nd_deduction_guides_test
| template <typename Value, unsigned int N> | ||
| blocked_nd_range(blocked_nd_range<Value, N>, oneapi::tbb::split) | ||
| -> blocked_nd_range<Value, N>; | ||
|
|
||
| template <typename Value, unsigned int N> | ||
| blocked_nd_range(blocked_nd_range<Value, N>, oneapi::tbb::proportional_split) | ||
| -> blocked_nd_range<Value, N>; | ||
|
|
There was a problem hiding this comment.
Splitting constructors are not really for users but for algorithms. Do we need deduction guides for these?
There was a problem hiding this comment.
Yes, I think the main purpose of the splitting constructors are TBB algorithms that don't currently use CTAD. But they are still part of the specification and the user may expect that for some implementation these constructors can be covered by the implicit deduction guides (see short example here).
And also all of the other blocked ranges in TBB currently supports CTAD for splitting constructors implicitly.
It seems like the reason why they do not work for N-d range are the class layout of the current implementation.
I feel that these deduction guides can be excluded from the future specification, but should be added into the implementation.
| template <typename Value, unsigned int N, | ||
| typename = std::enable_if_t<(N != 2 && N != 3)>> | ||
| blocked_nd_range(const Value (&)[N]) | ||
| -> blocked_nd_range<Value, N>; | ||
|
|
There was a problem hiding this comment.
What if the argument is explicitly an array of 2 or 3 elements, not a braced init list? Seems there is no deduction guide for it.
There was a problem hiding this comment.
Passing single array of 2 or 3 elements should be considered as a valid use-case, but having an extra deduction guide for array of 2 or 3 will also cover the braced-init-list of size 2 and 3.
As we discussed braced-init-list of size 2 and 3 should not be supported because of ambiguity between 1-D range constructed from single blocked_range and 2/3-D range constructed from the array.
This use-case is the main open question in the RFC #1607
There was a problem hiding this comment.
As discussed in the RFC, added support for both single array and single braced-init-list with preference to multi-dimensional ranges
| #if __TBB_CPP17_DEDUCTION_GUIDES_PRESENT | ||
| //! Testing blocked_rangeNd deduction guides | ||
| //! \brief \ref interface | ||
| TEST_CASE("blocked_rangeNd deduction guides") { |
There was a problem hiding this comment.
The test misses any checks for constructing explicitly from an array. It needs to check construction from arrays of different sizes (incl. 2 and 3), with and without a grainsize argument.
| oneapi::tbb::blocked_range<int> dim_range(0, 100); | ||
|
|
||
| blocked_nd_range<int, 2> source_range(dim_range, dim_range); |
There was a problem hiding this comment.
Can we test with something more fancy than int for the value type?
kboyarinov
changed the title
Description
Add a comprehensive description of proposed changes
Fixes # - issue number(s) if exists
Type of change
Choose one or multiple, leave empty if none of the other choices apply
Add a respective label(s) to PR if you have permissions
Tests
Documentation
Breaks backward compatibility
Notify the following users
List users with
@to send notificationsOther information