A hot fix for exclusive_scan

[MikeDvorskiy]

Summary

A hot fix for exclusive_scan (+ lost enable_if in declaration)
A negative test for exclusive_scan algorithm. It checks non-participation the algo declare in overload resolution.

[ldionne]

Do you have commit access?

[rodgert]

Actually, we should probably get a test of some sort here before accepting the change. Yes, it's fairly obviously wrong and the fix is fairly obviously correct, but it's a regression and we should provide tests for regressions, lest certain standard library maintainers ask uncomfortable questions.

[mclow.lists]

In D62719#1525450, @rodgert wrote:

Actually, we should probably get a test of some sort here before accepting the change. Yes, it's fairly obviously wrong and the fix is fairly obviously correct, but it's a regression and we should provide tests for regressions, lest certain standard library maintainers ask uncomfortable questions.

My ears are suddenly burning ;-)
Are people talking about me?

[MikeDvorskiy]

In D62719#1525424, @ldionne wrote:

Do you have commit access?

Yes, I've got it.

[MikeDvorskiy]

In D62719#1525450, @rodgert wrote:

Actually, we should probably get a test of some sort here before accepting the change. Yes, it's fairly obviously wrong and the fix is fairly obviously correct, but it's a regression and we should provide tests for regressions, lest certain standard library maintainers ask uncomfortable questions.

Do you mean a test should we added which will coverage the fixed error? In other words we need a test which checks following thing:

"Parallel algorithms shall not participate in overload resolution unless is_execution_policy_v<decay_-
t<ExecutionPolicy>> is true."

Right?

As far as I understand we should one call pear each algorithm with "fake" policy for is_execution_policy_v<fake_policy> is false and get a compilation error like "exclusive_scan(....).... is not found".
Otherwise, test "fail". So, it is "negative" test, right?
So, I have two questions:

1. Does LLVM test system support a negative test?
2. If yes, to cover the all cases (to be sure that "enable_if is present") we have to add more 80 negative test units.

Or we are talking about just a regression test which accepts the changes? In that case a regression test here is scan.pass.cpp.

[mclow.lists]

In D62719#1527089, @MikeDvorskiy wrote:

In D62719#1525450, @rodgert wrote:

Actually, we should probably get a test of some sort here before accepting the change. Yes, it's fairly obviously wrong and the fix is fairly obviously correct, but it's a regression and we should provide tests for regressions, lest certain standard library maintainers ask uncomfortable questions.

Do you mean a test should we added which will coverage the fixed error? In other words we need a test which checks following thing:

"Parallel algorithms shall not participate in overload resolution unless is_execution_policy_v<decay_-
t<ExecutionPolicy>> is true."

Right?

Right.

As far as I understand we should one call pear each algorithm with "fake" policy for is_execution_policy_v<fake_policy> is false and get a compilation error like "exclusive_scan(....).... is not found".
Otherwise, test "fail". So, it is "negative" test, right?
So, I have two questions:

1. Does LLVM test system support a negative test?

Yes it does. See (for example) "test/std/iterators/iterator.container/empty.array.fail.cpp"

2. If yes, to cover the all cases (to be sure that "enable_if is present") we have to add more 80 negative test units.

Agreed.

[rodgert]

In D62719#1525469, @mclow.lists wrote:

In D62719#1525450, @rodgert wrote:

Actually, we should probably get a test of some sort here before accepting the change. Yes, it's fairly obviously wrong and the fix is fairly obviously correct, but it's a regression and we should provide tests for regressions, lest certain standard library maintainers ask uncomfortable questions.

My ears are suddenly burning ;-)
Are people talking about me?

Hah, I didn't have you in mind specifically, but rather that other standard library maintainer that asked me about it.

[rodgert]

In D62719#1527360, @mclow.lists wrote:

In D62719#1527089, @MikeDvorskiy wrote:

In D62719#1525450, @rodgert wrote:

Actually, we should probably get a test of some sort here before accepting the change. Yes, it's fairly obviously wrong and the fix is fairly obviously correct, but it's a regression and we should provide tests for regressions, lest certain standard library maintainers ask uncomfortable questions.

Do you mean a test should we added which will coverage the fixed error? In other words we need a test which checks following thing:

"Parallel algorithms shall not participate in overload resolution unless is_execution_policy_v<decay_-
t<ExecutionPolicy>> is true."

Right?

Right.

As far as I understand we should one call pear each algorithm with "fake" policy for is_execution_policy_v<fake_policy> is false and get a compilation error like "exclusive_scan(....).... is not found".
Otherwise, test "fail". So, it is "negative" test, right?
So, I have two questions:

1. Does LLVM test system support a negative test?

Yes it does. See (for example) "test/std/iterators/iterator.container/empty.array.fail.cpp"

2. If yes, to cover the all cases (to be sure that "enable_if is present") we have to add more 80 negative test units.

Agreed.

I don't think we need all 80 tests to proceed with this commit, but here is as good a starting point as any on the process of adding such tests.

[MikeDvorskiy]

Added a negative test for exclusive_scan algorithm. It checks non-participation the algo declare in overload resolution.

[MikeDvorskiy]

+ fix filename in a file header

[ldionne]

test/std/numerics/numeric.ops/scan.fail.cpp

17 ↗

(On Diff #203315)

Do you really need this enable_if? I don't quite understand the utility of it.

[MikeDvorskiy]

test/std/numerics/numeric.ops/scan.fail.cpp

17 ↗

(On Diff #203315)

You right, enable_if looks redundant...
Please have a look at the updated patch.

[MikeDvorskiy]

1. The test was improved.
2. Minor changes in PSTL code. It needs for possibility of coverage that case in "negative test scenario".

[ldionne]

I'm committing this now, sorry it was lost in the weeds. I've made some minor fixes before committing, and I'm documenting them here for completeness.

test/CMakeLists.txt

26 ↗

(On Diff #204487)

This will make the tests fail cause pstl/ is not using the same lit configuration as libc++, for now it doesn't support fail tests. So I've removed this from the pstl tests, however when the libc++ test suite is run with pstl enabled, the test will be picked up.

test/std/numerics/numeric.ops/scan.fail.cpp

9 ↗

(On Diff #204487)

Added:

// UNSUPPORTED: c++98, c++03, c++11, c++14

13 ↗

(On Diff #204487)

Ran clang format.

25 ↗

(On Diff #204487)

Added (twice):

// expected-error {{no matching function for call to 'exclusive_scan'}}

include/pstl/internal/glue_numeric_defs.h

 // [exclusive.scan]
 
 template <class _ExecutionPolicy, class _ForwardIterator1, class _ForwardIterator2, class _Tp>
 __pstl::__internal::__enable_if_execution_policy<_ExecutionPolicy, _ForwardIterator2>
 exclusive_scan(_ExecutionPolicy&& __exec, _ForwardIterator1 __first, _ForwardIterator1 __last,
                _ForwardIterator2 __result, _Tp __init);
 
 template <class _ExecutionPolicy, class _ForwardIterator1, class _ForwardIterator2, class _Tp, class _BinaryOperation>
-_ForwardIterator2
+__pstl::__internal::__enable_if_execution_policy<_ExecutionPolicy, _ForwardIterator2>
 exclusive_scan(_ExecutionPolicy&& __exec, _ForwardIterator1 __first, _ForwardIterator1 __last,
                _ForwardIterator2 __result, _Tp __init, _BinaryOperation __binary_op);
 
 // [inclusive.scan]
 
 template <class _ExecutionPolicy, class _ForwardIterator1, class _ForwardIterator2>
 __pstl::__internal::__enable_if_execution_policy<_ExecutionPolicy, _ForwardIterator2>
 inclusive_scan(_ExecutionPolicy&& __exec, _ForwardIterator1 __first, _ForwardIterator1 __last,

include/pstl/internal/glue_numeric_impl.h

 exclusive_scan(_ExecutionPolicy&& __exec, _ForwardIterator1 __first, _ForwardIterator1 __last,
                _ForwardIterator2 __result, _Tp __init)
 {
     return transform_exclusive_scan(std::forward<_ExecutionPolicy>(__exec), __first, __last, __result, __init,
                                     std::plus<_Tp>(), __pstl::__internal::__no_op());
 }
 
 template <class _ExecutionPolicy, class _ForwardIterator1, class _ForwardIterator2, class _Tp, class _BinaryOperation>
-_ForwardIterator2
+__pstl::__internal::__enable_if_execution_policy<_ExecutionPolicy, _ForwardIterator2>
 exclusive_scan(_ExecutionPolicy&& __exec, _ForwardIterator1 __first, _ForwardIterator1 __last,
                _ForwardIterator2 __result, _Tp __init, _BinaryOperation __binary_op)
 {
     return transform_exclusive_scan(std::forward<_ExecutionPolicy>(__exec), __first, __last, __result, __init,
                                     __binary_op, __pstl::__internal::__no_op());
 }
 
 // [inclusive.scan]