[libcxx-commits] [PATCH] D120186: [pstl] Implementation of Grand Central Dispatch backend

[Christopher Nelson via Phabricator via libcxx-commits]

nadiasvertex marked 2 inline comments as done.
nadiasvertex added inline comments.


================
Comment at: pstl/include/pstl/internal/gcd/parallel_invoke.h:38
+                         {
+                             std::get<1> (*__d)();
+                         }
----------------
Quuxplusone wrote:
> Here and line 34, extraneous whitespace. (Weird spacing throughout, honestly, but that's clang-format for you.)
I have re-run clang-format on the code. Not sure what else to do about it.


================
Comment at: pstl/include/pstl/internal/gcd/parallel_merge.h:70
+   */
+    __parallel_merge_body(__size_x, __size_y, __xs, __xe, __ys, __ye, __zs, __comp, __leaf_merge);
+}
----------------
Quuxplusone wrote:
> Here and throughout, FYI, the code is not ADL-proof, which means it'll have this problem:
> https://quuxplusone.github.io/blog/2019/09/26/uglification-doesnt-stop-adl/
> I'm sure this is a pre-existing problem throughout PSTL, though. (We try to avoid it in libc++ proper but I assume nobody's ever made a big deal about it in PSTL.)
Good point. I've tried to fix this everywhere, in the same way that the OpenMP backend fixes it.


================
Comment at: pstl/include/pstl/internal/gcd/parallel_stable_sort.h:109
+{
+    assert(__xe > __xs);
+
----------------
Quuxplusone wrote:
> (1) It kinda looks like `__pattern_sort` (`pstl/include/pstl/internal/algorithm_impl.h` line 2198) can call this with `__xe == __xs`. But maybe I'm wrong.
> (2) `_PSTL_ASSERT`?
Good catch, thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120186