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

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

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

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

10

Added:

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

14

Ran clang format.

26

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]

test/CMakeLists.txt

add_custom_target(check-pstl
     DEPENDS pstl-build-tests
     COMMENT "Build and run all the unit tests.")
 
 add_library(test_stdlib INTERFACE)
 target_include_directories(test_stdlib INTERFACE "${CMAKE_CURRENT_SOURCE_DIR}/support/stdlib")
 target_link_libraries(test_stdlib INTERFACE pstl::ParallelSTL)
 target_compile_options(test_stdlib INTERFACE -Wno-gnu-include-next)
 
-file(GLOB_RECURSE UNIT_TESTS "*.pass.cpp")
+file(GLOB_RECURSE UNIT_TESTS "*.pass.cpp" "*.fail.cpp")
 foreach(_file IN LISTS UNIT_TESTS)
     file(RELATIVE_PATH _target "${CMAKE_CURRENT_SOURCE_DIR}" "${_file}")
     string(REPLACE ".cpp" "" _target "${_target}")
     string(REPLACE "/" "-" _target "${_target}")
     set(_target "pstl-${_target}")
 
     add_executable(${_target} EXCLUDE_FROM_ALL "${_file}")
     target_include_directories(${_target} PRIVATE "${CMAKE_CURRENT_LIST_DIR}")
     target_compile_options(${_target} PRIVATE -Wno-unused-local-typedef -Wno-unused-variable)
     target_link_libraries(${_target} PRIVATE test_stdlib)
     set_target_properties(${_target} PROPERTIES CXX_EXTENSIONS NO
                                                 RUNTIME_OUTPUT_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}")
     add_test(${_target} "${CMAKE_CURRENT_BINARY_DIR}/${_target}")
     add_dependencies(pstl-build-tests ${_target})
 endforeach()

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
+//
+//===----------------------------------------------------------------------===//
+
+#include <execution>
+#include <numeric>
+
+class CustomPolicy {
+};
+
+template<typename PolicyType>
+typename std::enable_if<!std::is_execution_policy<PolicyType>::value, void>::type
+run_algo() {
+
+    CustomPolicy policy;
+    int *first = nullptr, *last=nullptr, *result = nullptr;
+
+    std::exclusive_scan(policy, first, last, result, 0);
+    std::exclusive_scan(policy, first, last, result, 0, std::plus<int>());
+}
+
+template<typename PolicyType>
+typename std::enable_if<std::is_execution_policy<PolicyType>::value, void>::type
+run_algo() {}
+
+int32_t
+main()
+{    
+    run_algo<CustomPolicy>();
+    return 0;
+}