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'}}

pstl/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,

pstl/include/pstl/internal/glue_numeric_impl.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)
 {
-    return transform_exclusive_scan(std::forward<_ExecutionPolicy>(__exec), __first, __last, __result, __init,
-                                    std::plus<_Tp>(), __pstl::__internal::__no_op());
+    using namespace __pstl;
+    return __internal::__pattern_transform_scan(
+        std::forward<_ExecutionPolicy>(__exec), __first, __last, __result, __pstl::__internal::__no_op(), __init,
+        std::plus<_Tp>(), /*inclusive=*/std::false_type(),
+        __internal::__is_vectorization_preferred<_ExecutionPolicy, _ForwardIterator1, _ForwardIterator2>(__exec),
+        __internal::__is_parallelization_preferred<_ExecutionPolicy, _ForwardIterator1, _ForwardIterator2>(__exec));
 }
 
 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());
+    using namespace __pstl;
+    return __internal::__pattern_transform_scan(
+        std::forward<_ExecutionPolicy>(__exec), __first, __last, __result, __pstl::__internal::__no_op(), __init,
+        __binary_op, /*inclusive=*/std::false_type(),
+        __internal::__is_vectorization_preferred<_ExecutionPolicy, _ForwardIterator1, _ForwardIterator2>(__exec),
+        __internal::__is_parallelization_preferred<_ExecutionPolicy, _ForwardIterator1, _ForwardIterator2>(__exec));
 }
 
 // [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,
                _ForwardIterator2 __result)

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

+// -*- C++ -*-
+//===-- scan.fail.cpp -----------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++98, c++03, c++11, c++14
+
+#include <execution>
+#include <numeric>
+
+struct CustomPolicy
+{
+    constexpr std::false_type
+    __allow_vector()
+    {
+        return std::false_type{};
+    }
+    constexpr std::false_type
+    __allow_parallel()
+    {
+        return std::false_type{};
+    }
+} policy;
+
+int32_t
+main()
+{
+    int *first = nullptr, *last = nullptr, *result = nullptr;
+
+    std::exclusive_scan(policy, first, last, result, 0); // expected-error {{no matching function for call to 'exclusive_scan'}}
+    std::exclusive_scan(policy, first, last, result, 0, std::plus<int>()); // expected-error {{no matching function for call to 'exclusive_scan'}}
+
+    return 0;
+}