Wrap all functions dispatched by a parallel policy tag in a conditional directive.
Diff Detail
Event Timeline
include/pstl/internal/algorithm_impl.h | ||
---|---|---|
62–75 | What if I suggest excluding the overloaded parallel pattern (here and everywhere) by SFINAE approach, like this: pstl::internal::enable_if_execution_policy<_ExecutionPolicy, bool> The macro __PSTL_USE_PAR_POLICIES controls parallel policies definitions and it should be enough. |
include/pstl/internal/algorithm_impl.h | ||
---|---|---|
62–75 | Mikhail, I think that use of enable_if_execution_policy is inconsistent if only applied to "parallel" overloads, and less clear comparing to the macro. If the SFINAE way is preferred, we should make it differently, e.g. introduce enable_if_parallel/enable_if_not_parallel and use those instead of is_parallel function parameter which becomes redundant. |
LGTM. I can witness the problem locally with the CMake build and the fix resolves the issue. Thanks!
Committed as r350813.
See inline comment
include/pstl/internal/algorithm_impl.h | ||
---|---|---|
181 | Shouldn't this definition be protected by #ifdef __PSTL_USE_PAR_POLICIES? Sure, it will fail to compile because it calls something that is __PSTL_USE_PAR_POLICIES but what was it ldionne said about being consistent recently? |
See inline notes on __PSTL_USE_PAR_POLICIES #ifdefs
include/pstl/internal/algorithm_impl.h | ||
---|---|---|
2432–2439 | Shouldn't this definition also be protected by #ifdef __PSTL_USE_PAR_POLICIES? |
Added a reply note to Mihail and Alexey's inline commentary on whether __PSTL_USE_PAR_POLICIES is how we really want to be controlling this.
include/pstl/internal/algorithm_impl.h | ||
---|---|---|
181 | I don't understand your comment. What is inconsistent about pattern_walk1_n not being guarded by #ifdef __PSTL_USE_PAR_POLICIES? Sorry, just trying to understand. FWIW, I think that removing code based on __PSTL_USE_PAR_POLICIES is a misfeature and we should find a more gracious way of handling the lack of parallel policies, for example by having a backend that does everything sequentially if that makes sense. I accepted this revision because it fixed a compilation error under a configuration that currently seems to be supported. |
include/pstl/internal/algorithm_impl.h | ||
---|---|---|
181 | Sorry, I applied the inline comment to the wrong variant of pattern_walk1_n, it's the next one down, where /*is_parallel=*/std::true_type I also agree that removing code based on __PSTL_USE_PAR_POLICIES is a misfeature. I want to propose some alternatives based on work I've started on an OpenMP based backend and discussions that came out of SG1 in San Diego about likely future directions with this part of the standard library, but I need to finish this merge and get a patch into libstdc++ before I can return to that work. In doing the reconciliation with the libstdc++ version of this code with what was submitted upstream (which unfortunately differs far more than what I thought we had agreed for the initial commit), I came across these inconsistencies and am just reporting them (they are fixed in my local branch, and will be part of that submission when it's ready). | |
2506–2515 | This should also be protected by #ifdef __PSTL_USE_PAR_POLICIES |
include/pstl/internal/algorithm_impl.h | ||
---|---|---|
181 | I addressed you comment in this new change: https://reviews.llvm.org/D57638 |
What if I suggest excluding the overloaded parallel pattern (here and everywhere) by SFINAE approach, like this:
pstl::internal::enable_if_execution_policy<_ExecutionPolicy, bool>
pattern_any_of(_ExecutionPolicy&& exec, _ForwardIterator first, _ForwardIterator last, _Pred __pred,.....
The macro __PSTL_USE_PAR_POLICIES controls parallel policies definitions and it should be enough.