[libcxx-commits] [PATCH] D60480: [libc++] Integrate the PSTL into libc++

[Louis Dionne via Phabricator via libcxx-commits]

ldionne marked 4 inline comments as done.
ldionne added a comment.

Please note that this review is really about integrating pstl into libc++, not pstl itself. I know there are changed needed before pstl is ready for prime time (and `LIBCXX_ENABLE_PARALLEL_ALGORITHMS` becomes `ON` by default).



================
Comment at: libcxx/include/algorithm:5790
+// otherwise just pull in forward declarations.
+#include <pstl/internal/pstl_config.h>
+#if defined(__PSTL_EXECUTION_POLICIES_DEFINED)
----------------
EricWF wrote:
> ldionne wrote:
> > The same pattern is repeated in other files. Basically, we check whether `<execution>` has been included, and if so, then we provide all the definitions. Otherwise, we just provide forward declarations to avoid the compile-time overhead if you just `#include <algorithm>` but don't care about the parallel algorithms.
> > 
> I really really really dislike this pattern. We shouldn't do it.
> I don't think this is how you should structure a library.
> 
> Are there other alternatives?
I dislike it too. The only alternative I can think of is to unconditionally include the definitions, however this may impose a compile-time hit on everyone that's not using the parallel algorithms.


================
Comment at: libcxx/include/algorithm:5791
+#include <pstl/internal/pstl_config.h>
+#if defined(__PSTL_EXECUTION_POLICIES_DEFINED)
+#    include <pstl/internal/glue_algorithm_impl.h>
----------------
EricWF wrote:
> We normally use `_ONE_UNDERSCORE_UPPER` or `__two_underscore_lower`
For file names? Please disregard pstl file names for the time being. The goal of this review is to check-in the integration between libc++ and pstl, but it's understood that pstl's structure is going to change in the future. I'm just trying to integrate early to better see how changes in pstl will end up impacting libc++.


================
Comment at: libcxxabi/src/CMakeLists.txt:149
 endif()
 
+find_package(ParallelSTL)
----------------
EricWF wrote:
> Why does libc++abi depend on parallel algorithms? That seems backwards?
Yes, it is. It's because libc++abi includes headers from libc++ that pull in the parallel algorithms when they're enabled. So this is needed to support building libc++abi against a libc++ that has the parallel algorithms enabled.


================
Comment at: pstl/test/pstl/header_inclusion_order_algorithm_0.pass.cpp:11
 // UNSUPPORTED: c++98, c++03, c++11, c++14
+// REQUIRES: parallel-algorithms
 
----------------
EricWF wrote:
> Does every test under `pstl/test/pstl` require "parallel-algorithms"? Because if so there are easier ways to do this.
Yes, they all do. I could also use a `lit.local.cfg` if that's what you're thinking about?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60480