[libcxx-commits] [PATCH] D99836: [pstl] Implement OpenMP backend

[Christopher Nelson via Phabricator via libcxx-commits]

nadiasvertex marked 6 inline comments as done.
nadiasvertex added a comment.

Resolved all outstanding comments.



================
Comment at: pstl/include/pstl/internal/omp/parallel_for_each.h:27
+    // TODO: Think of an approach to remove the std::distance call
+    auto __size = ::std::distance(__first, __last);
+
----------------
ldionne wrote:
> It should be sufficient to use `std::distance`, not `::std::distance`. Any reason why you're doing the latter? Applies in a bunch of places.
Someone mentioned for ADL it is better to root it in the global space. I am fine with taking it out.


================
Comment at: pstl/include/pstl/internal/omp/util.h:28
+#if !defined(_OPENMP)
+#    error _OPENMP not defined.
+#endif
----------------
ldionne wrote:
> This error message isn't too helpful. Something like this would probably be better:
> 
> ```
> #if !defined(_OPENMP)
> #    error The OpenMP PSTL backend requires OpenMP, but it looks like your platform doesn't implement it.
> #endif
> ```
> 
> Is the above message accurate and what you're trying to convey to the user? Also, is there any way that you'd be able to include `<omp.h>` yet OpenMP wouldn't be available? If not, then the `#if !defined(_OPENMP)` block is useless altogether.
> 
Probably true. This was reduced from a larger block that checked various openmp versions. I will remove it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99836/new/

https://reviews.llvm.org/D99836