-
Notifications
You must be signed in to change notification settings - Fork 1.6k
<execution>, <numeric>: Change improper direct-initialization to copy-initialization in C++17 numeric algorithms
#4419
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
Merged
Stephan T. Lavavej (StephanTLavavej)
merged 11 commits into
microsoft:main
from
frederick-vs-ja:fix-parallel-numeric-conv
Feb 29, 2024
Merged
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
9e6e2dd
Fix improper direct-initialization in numeric algorithms
frederick-vs-ja e84c2d7
Missed Clang-format
frederick-vs-ja 3f811a9
Try to fix old and new tests
frederick-vs-ja e2b2cc2
Drop mistaken case for `transform_inclusive_scan`
frederick-vs-ja f23115b
Revert the removal by restoration
frederick-vs-ja eb360df
Remove an accidentally inserted U+3000
frederick-vs-ja f725690
Merge branch 'microsoft:main' into fix-parallel-numeric-conv
frederick-vs-ja 48e4702
Fix typos: tranform => transform (preserving case)
StephanTLavavej 685f457
Fix typos: implicity => implicitly
StephanTLavavej 7ef665c
Fix typo: form => from
StephanTLavavej bb3845a
Take ops by value - they're always `_Pass_fn`.
StephanTLavavej File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
4 changes: 4 additions & 0 deletions
4
tests/std/tests/GH_004129_conversion_in_new_numeric_algorithms/env.lst
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| # Copyright (c) Microsoft Corporation. | ||
| # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | ||
|
|
||
| RUNALL_INCLUDE ..\usual_17_matrix.lst |
186 changes: 186 additions & 0 deletions
186
tests/std/tests/GH_004129_conversion_in_new_numeric_algorithms/test.cpp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,186 @@ | ||
| // Copyright (c) Microsoft Corporation. | ||
| // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | ||
|
|
||
| // intentionally test narrowing conversion from int64_t to int32_t | ||
| #pragma warning(disable : 4244) | ||
|
|
||
| #include <cassert> | ||
| #include <cstdint> | ||
| #include <execution> | ||
| #include <numeric> | ||
| #include <type_traits> | ||
|
|
||
| using namespace std; | ||
| using namespace std::execution; | ||
|
|
||
| struct implicitly_convertible_to_i32_only { | ||
| int32_t n; | ||
|
|
||
| template <class = void> | ||
| explicit operator int32_t() const = delete; | ||
|
|
||
| operator int64_t() const noexcept { | ||
| return n; | ||
| } | ||
| }; | ||
|
|
||
| static_assert(!is_constructible_v<int32_t, implicitly_convertible_to_i32_only>); | ||
| static_assert(is_convertible_v<implicitly_convertible_to_i32_only, int32_t>); | ||
|
|
||
| struct implicitly_validating_converter { | ||
| implicitly_convertible_to_i32_only operator()(int n) const noexcept { | ||
| return {n}; | ||
| } | ||
| }; | ||
|
|
||
| struct explicitly_convertible_to_i32_only { | ||
| int32_t n; | ||
|
|
||
| explicit operator int32_t() const noexcept { | ||
| return n; | ||
| } | ||
| }; | ||
|
|
||
| static_assert(is_constructible_v<int32_t, explicitly_convertible_to_i32_only>); | ||
| static_assert(!is_convertible_v<explicitly_convertible_to_i32_only, int32_t>); | ||
|
|
||
| struct transformation_validating_converter { | ||
| explicitly_convertible_to_i32_only operator()(int n) const noexcept { | ||
| return {n}; | ||
| } | ||
| }; | ||
|
|
||
| struct implicitly_validating_plus { | ||
| implicitly_convertible_to_i32_only operator()( | ||
| implicitly_convertible_to_i32_only l, implicitly_convertible_to_i32_only r) const noexcept { | ||
| return implicitly_convertible_to_i32_only{l.n + r.n}; | ||
| } | ||
|
|
||
| implicitly_convertible_to_i32_only operator()(int32_t l, implicitly_convertible_to_i32_only r) const noexcept { | ||
| return implicitly_convertible_to_i32_only{l + r.n}; | ||
| } | ||
|
|
||
| implicitly_convertible_to_i32_only operator()(implicitly_convertible_to_i32_only l, int32_t r) const noexcept { | ||
| return implicitly_convertible_to_i32_only{l.n + r}; | ||
| } | ||
|
|
||
| implicitly_convertible_to_i32_only operator()(int32_t l, int32_t r) const noexcept { | ||
| return implicitly_convertible_to_i32_only{l + r}; | ||
| } | ||
| }; | ||
|
|
||
| struct implicitly_validating_plus_for_transformation { | ||
| implicitly_convertible_to_i32_only operator()( | ||
| explicitly_convertible_to_i32_only l, explicitly_convertible_to_i32_only r) const noexcept { | ||
| return implicitly_convertible_to_i32_only{l.n + r.n}; | ||
| } | ||
|
|
||
| implicitly_convertible_to_i32_only operator()(int32_t l, explicitly_convertible_to_i32_only r) const noexcept { | ||
| return implicitly_convertible_to_i32_only{l + r.n}; | ||
| } | ||
|
|
||
| implicitly_convertible_to_i32_only operator()(explicitly_convertible_to_i32_only l, int32_t r) const noexcept { | ||
| return implicitly_convertible_to_i32_only{l.n + r}; | ||
| } | ||
|
|
||
| implicitly_convertible_to_i32_only operator()(int32_t l, int32_t r) const noexcept { | ||
| return implicitly_convertible_to_i32_only{l + r}; | ||
| } | ||
| }; | ||
|
|
||
| struct implicitly_validating_multiplies { | ||
| implicitly_convertible_to_i32_only operator()( | ||
| implicitly_convertible_to_i32_only l, implicitly_convertible_to_i32_only r) const noexcept { | ||
| return implicitly_convertible_to_i32_only{l.n * r.n}; | ||
| } | ||
|
|
||
| implicitly_convertible_to_i32_only operator()(int32_t l, implicitly_convertible_to_i32_only r) const noexcept { | ||
| return implicitly_convertible_to_i32_only{l * r.n}; | ||
| } | ||
|
|
||
| implicitly_convertible_to_i32_only operator()(implicitly_convertible_to_i32_only l, int32_t r) const noexcept { | ||
| return implicitly_convertible_to_i32_only{l.n * r}; | ||
| } | ||
|
|
||
| implicitly_convertible_to_i32_only operator()(int32_t l, int32_t r) const noexcept { | ||
| return implicitly_convertible_to_i32_only{l * r}; | ||
| } | ||
| }; | ||
|
|
||
| void test_copy_initialization_for_numeric_algorithms() { | ||
| int arr[1]{}; | ||
| implicitly_convertible_to_i32_only brr[1]{}; | ||
|
|
||
| assert(reduce(arr, arr, int32_t{}, implicitly_validating_plus{}) == 0); | ||
| assert(reduce(brr, brr, int32_t{}, implicitly_validating_plus{}) == 0); | ||
|
|
||
| assert(transform_reduce(arr, arr, arr, int32_t{}, implicitly_validating_plus{}, implicitly_validating_multiplies{}) | ||
| == 0); | ||
| assert(transform_reduce(arr, arr, brr, int32_t{}, implicitly_validating_plus{}, implicitly_validating_multiplies{}) | ||
| == 0); | ||
|
|
||
| assert(transform_reduce(arr, arr, int32_t{}, implicitly_validating_plus{}, implicitly_validating_converter{}) == 0); | ||
| assert(transform_reduce(brr, brr, int32_t{}, implicitly_validating_plus{}, implicitly_validating_converter{}) == 0); | ||
|
|
||
| assert(exclusive_scan(arr, arr, arr, int32_t{}, implicitly_validating_plus{}) == arr); | ||
|
|
||
| assert(inclusive_scan(arr, arr, arr, implicitly_validating_plus{}) == arr); | ||
| assert(inclusive_scan(arr, arr, arr, implicitly_validating_plus{}, int32_t{}) == arr); | ||
|
|
||
| assert(transform_exclusive_scan(arr, arr, arr, int32_t{}, implicitly_validating_plus_for_transformation{}, | ||
| transformation_validating_converter{}) | ||
| == arr); | ||
|
|
||
| assert(transform_inclusive_scan(brr, brr, brr, implicitly_validating_plus{}, implicitly_validating_converter{}) | ||
| == brr); | ||
| assert(transform_inclusive_scan(arr, arr, arr, implicitly_validating_plus_for_transformation{}, | ||
| transformation_validating_converter{}, int32_t{}) | ||
| == arr); | ||
| } | ||
|
|
||
| template <const auto& ExPo> | ||
| void test_copy_initialization_for_parallel_numeric_algorithms() { | ||
| int arr[1]{}; | ||
| implicitly_convertible_to_i32_only brr[1]{}; | ||
|
|
||
| assert(reduce(ExPo, arr, arr, int32_t{}, implicitly_validating_plus{}) == 0); | ||
| assert(reduce(ExPo, brr, brr, int32_t{}, implicitly_validating_plus{}) == 0); | ||
|
|
||
| assert(transform_reduce( | ||
| ExPo, arr, arr, arr, int32_t{}, implicitly_validating_plus{}, implicitly_validating_multiplies{}) | ||
| == 0); | ||
| assert(transform_reduce( | ||
| ExPo, arr, arr, brr, int32_t{}, implicitly_validating_plus{}, implicitly_validating_multiplies{}) | ||
| == 0); | ||
|
|
||
| assert(transform_reduce(ExPo, arr, arr, int32_t{}, implicitly_validating_plus{}, implicitly_validating_converter{}) | ||
| == 0); | ||
| assert(transform_reduce(ExPo, brr, brr, int32_t{}, implicitly_validating_plus{}, implicitly_validating_converter{}) | ||
| == 0); | ||
|
|
||
| assert(exclusive_scan(ExPo, arr, arr, arr, int32_t{}, implicitly_validating_plus{}) == arr); | ||
|
|
||
| assert(inclusive_scan(ExPo, arr, arr, arr, implicitly_validating_plus{}) == arr); | ||
| assert(inclusive_scan(ExPo, arr, arr, arr, implicitly_validating_plus{}, int32_t{}) == arr); | ||
|
|
||
| assert(transform_exclusive_scan(ExPo, arr, arr, arr, int32_t{}, implicitly_validating_plus_for_transformation{}, | ||
| transformation_validating_converter{}) | ||
| == arr); | ||
|
|
||
| assert( | ||
| transform_inclusive_scan(ExPo, brr, brr, brr, implicitly_validating_plus{}, implicitly_validating_converter{}) | ||
| == brr); | ||
| assert(transform_inclusive_scan(ExPo, arr, arr, arr, implicitly_validating_plus_for_transformation{}, | ||
| transformation_validating_converter{}, int32_t{}) | ||
| == arr); | ||
| } | ||
|
|
||
| int main() { | ||
| test_copy_initialization_for_numeric_algorithms(); | ||
| test_copy_initialization_for_parallel_numeric_algorithms<seq>(); | ||
| test_copy_initialization_for_parallel_numeric_algorithms<par>(); | ||
| test_copy_initialization_for_parallel_numeric_algorithms<par_unseq>(); | ||
| #if _HAS_CXX20 | ||
| test_copy_initialization_for_parallel_numeric_algorithms<unseq>(); | ||
| #endif // _HAS_CXX20 | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 capturing it by reference too. Ditto below.
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.
The point is performing implicit conversion in placement new with workaround for imperfectness of forwarding. So I guess copying should be avoided.
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'm fine with capturing by reference since it doesn't affect the function signature, even though it should be a reference to a
_Pass_fn.