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

[Richard Smith - zygoloid via Phabricator via libcxx-commits]

rsmith added inline comments.


================
Comment at: libcxx/include/algorithm:5790
+// otherwise just pull in forward declarations.
+#include <pstl/internal/pstl_config.h>
+#if defined(__PSTL_EXECUTION_POLICIES_DEFINED)
----------------
dexonsmith wrote:
> ldionne wrote:
> > 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.
> @bruno, will this pattern cause a problem for Clang modules (esp. with local submodule visibility)?
This won't work with modules (neither Clang modules with local submodule visibility, nor C++20 modules). We will build `<algorithm>` in a context where `<execution>` has not been included, and vice versa, and neither of them will contain definitions of the parallel algorithms. More fundamentally, we should not be trying to create a situation where we have two headers and including both of them together provides more content than the union of the content provided by including the two separately -- that's bad header design and inevitably will lead to problems.

One possible alternative design approach:

 * `<algorithm>` provides definitions of the algorithms that forward to `ExecutionPolicy::__<algorithm>(<args>)` if they exist, or otherwise forward to the non-execution-policy overload if `ExecutionPolicy::<some tag member>` exists (indicating that the template argument was a real execution policy that just doesn't happen to specialize that algorithm), and otherwise produce an error that the argument is not an execution policy.
 * `<execution>` defines the specialized algorithms for each execution policy as static member functions on the execution policy.

This adds minimal cost to code that only includes `<algorithm>` and not `<execution>` while keeping both headers modular. It also supports the easy addition of new implementation-specific execution policies, defining one policy as a refinement of another, and gives an easy way for an implementation to be conforming in the absence of a `pstl` implementation (or to provide an easy non-parallel implementation for single-threaded builds): just define the execution policy objects with no members other than the "I'm a real execution policy" tag member.

Would that work?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60480