Implement P2248R8: Enable list-initialization for algorithms #2398
Conversation
rarutyun
requested review from
SergeyKopienko,
adamfidel,
akukanov,
danhoeflinger,
dmitriy-sobolev and
mmichel11
Yes. Let's do that for all algorithms, including - if necessary - non-standard ones in oneDPL. |
|
Is the intention to merge this for milestone 2022.10.0? |
Whenever we think it's ready |
Tests for hetero backend remain
rarutyun
force-pushed
the
rarutyun/implement_p2248r8
branch
from
d0441d2 to
f1b47c6
Compare
| _ForwardIterator2 __result, _UnaryPredicate __pred, const _Tp& __new_value); | ||
|
|
||
| template <class _ExecutionPolicy, class _ForwardIterator1, class _ForwardIterator2, class _Tp> | ||
| template <class _ExecutionPolicy, class _ForwardIterator1, class _ForwardIterator2, |
There was a problem hiding this comment.
Doesn't the paper conclude not to make any changes to replace_copy for non ranges api?
danhoeflinger
left a comment
danhoeflinger
left a comment
There was a problem hiding this comment.
Other than the question on replace_copy, I don't see any issues with the implementation, but I have not yet checked the tests.
| EXPECT_TRUE(std::count(v.begin(), v.end(), 0) == v.size(), "a sequence is not filled properly by oneapi::dpl::fill with `unseq` policy"); | ||
| } | ||
|
|
||
| { |
There was a problem hiding this comment.
[nitpick] Remove extra unnecessary braces (this goes away if we use invoke_on_all_policies)
There was a problem hiding this comment.
As discussed offline, these tests (for replace_copy, but not replace_copy_if) should be removed along with the default template arg for replace_copy in the implementation.
| static_assert(is_replace_copy_if_well_formed<std::vector<not_implicitly_convertible>::iterator, std::vector<int>::iterator>::value, | ||
| "Positive: The default argument of replace_copy_if shall be taken from output iterator"); | ||
| static_assert(!is_replace_copy_if_well_formed<std::vector<int>::iterator, std::vector<not_implicitly_convertible>::iterator>::value, | ||
| "Positive: The default argument of replace_copy_if shall be taken from output iterator"); |
There was a problem hiding this comment.
The "Positive" and "Negative" here are confusing for me (and the one on like 150 should be "Negative" I think).
I'd prefer something more like:
"replace_copy_if call is NOT well-formed for valid list_initialization argument"
and
"replace_copy_if call is well-formed for INVALID list_initialization argument"
Or something similar so that it is clear to the user reading the error message what has gone wrong without looking at the tests.
| // Test with value that is equal to two different bit patterns (-0.0 and 0.0) | ||
| test<float32_t>(-0.0, [](std::int32_t j) { return j & 1 ? 0.0 : -0.0; }, // hit | ||
| [](std::int32_t j) { return j == 0 ? ~j : j; }); // miss | ||
| [](std::int32_t j) { return j == 0 ? ~j : j; }); // miss |
There was a problem hiding this comment.
It seems like these are bad formatting changes that should be reverted.
| sycl::buffer<int> buf(v); | ||
| auto it = oneapi::dpl::search_n(oneapi::dpl::execution::dpcpp_default, oneapi::dpl::begin(buf), oneapi::dpl::end(buf), {}); | ||
| EXPECT_TRUE(it.get_idx() == 4, "an empty list-initialized value is not found by oneapi::dpl::search_n with `device_policy` policy"); |
There was a problem hiding this comment.
[nitpick] Is there a reason to only test the integer variant on dpcpp_default rather that both integer and DefaultInitializedToOne?
I know that from a white box perspective, all these policies are really testing the same part of the code as they share code at the public API level, but for the other policy types we test both.
Same question for the other tests as well.
3 participants
The link to the proposal
Implementing Iterator-based algorithms only. Ranges-based implementation is going on separately.
There are no breaking changes. All the breaking changes in C++ standard come into
std::rangesnamespace but even they are irrelevant to oneDPL because we implement list-initialization enabling right away, so there is no template parameters swappingThere are two oversights for P2248:
find_lastwas not included (see this paper). However, it's irrelevant for Iterator-based algorithms becausefind_lastexists instd::ranges:namespace onlyuninitialized_fillwas not included (see this paper. However, it's not included for C++26 yet. I've implemented that proactively as was requested. There is a NB comment to include this oversight for C++26.Binary Search-like algorithms from P2248 (e.g.,
lower_bound,upper_bound, etc.) are not relevant either because our API for those does not haveT-like template parameter.Linked issue: #1508