This is an archive of the discontinued LLVM Phabricator instance.

A hot fix for exclusive_scan
ClosedPublic

Authored by MikeDvorskiy on May 31 2019, 1:38 AM.

Details

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.

Diff Detail

Repository
rPSTL pstl

Event Timeline

MikeDvorskiy created this revision.May 31 2019, 1:38 AM
rodgert accepted this revision.May 31 2019, 10:55 AM
This revision is now accepted and ready to land.May 31 2019, 10:55 AM
ldionne accepted this revision.May 31 2019, 11:11 AM

Do you have commit access?

rodgert requested changes to this revision.May 31 2019, 12:16 PM

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.

This revision now requires changes to proceed.May 31 2019, 12:16 PM

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?

Do you have commit access?

Yes, I've got it.

MikeDvorskiy added a comment.EditedJun 3 2019, 1:48 AM

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.

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"

  1. 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.

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.

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"

  1. 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 edited the summary of this revision. (Show Details)

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

+ fix filename in a file header

ldionne added inline comments.Jun 6 2019, 4:29 PM
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 marked an inline comment as done.Jun 13 2019, 4:53 AM
MikeDvorskiy added inline comments.
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.

  1. The test was improved.
  2. Minor changes in PSTL code. It needs for possibility of coverage that case in "negative test scenario".
ldionne accepted this revision.Mar 17 2020, 1:01 PM

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'}}
This revision was not accepted when it landed; it landed in state Needs Review.Mar 17 2020, 1:30 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 17 2020, 1:30 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript