[libcxx-commits] [PATCH] D141779: [libc++][PSTL] Copy the headers into libc++

[Ruslan Arutyunyan via Phabricator via libcxx-commits]

rarutyun added a comment.

Thanks for keeping me in the loop.

In D141779#4189841 <https://reviews.llvm.org/D141779#4189841>, @ldionne wrote:

> 2. No more `<pstl/internal/algorithm_impl.h>` and `<pstl/internal/algorithm_fwd.h>` headers & friends -- those contain the implementation of algorithms and they would be in `<__algorithm/pstl_any_of.h>` & friends instead.
> 3. No more `<pstl/internal/glue_algorithm_defs.h>` and `<pstl/internal/glue_algorithm_impl.h>` headers & friends -- those contain the standard facing algorithm names, which would now be directly in `<__algorithm/pstl_any_of.h>` & friends.
> 4. The headers implementing backends are kept and we try not to make significant changes to them, if possible. This should allow keeping the backends in sync with `pstl/` as needed. We might want to reorganize them under something like `<__pstl_backends/{omp.h,tbb.h,serial.h,etc.}>` or something similar.

While I understand the approach you suggested for `glue_algorithm_<fwd|impl>.h` (and friends) I think that `algorithm_<fwd|impl>.h` (and friends) is something you probably want to keep more or less as is. They might be reorganized similar to what you suggested for `__pstl_backends` but in general `algorithm_impl.h` contains patterns, public API has mapping on. Since many different algorithms could map on single pattern it's common layer and I am not sure that you want to have that layer split between the files that correspond to public API. If I am not mistaken it might be not so simple to do that from both

- separation perspective (you would eventually need to include those that might create undesired algorithm dependencies)
- further synchronization with PSTL upstream perspective.

Probably the good thing to do is a renaming of `algorithm_impl.h` and similar ones to something that would not confuse libc++ people (e.g., `__pstl[_parallel]_patterns/something`).

The choice is yours, anyway.

> 6. The current `pstl/` has several layers of forward declarations and boilerplate, and I think we can simplify that since we don't need the flexibility of e.g. `std::any_of(ExecutionPolicy, ...)` being forward-declared anymore. So as you iterate to achieve the above steps (in particular 1-2-3), it would be reasonable to fold some of the layers of complexity. I see you've done that already in e.g. D143161 <https://reviews.llvm.org/D143161>.

The only reason why forwarding declarations were introduced is reducing the compilation time when parallel backends (i.e.., both TBB and OpenMP) are not available or if parallel policies (`par`, `par_unseq`) are disabled explicitly (if this feature is supposed to be at all. For example, with defining some macro). If you guys think that it's something that impossible in your case then you probably don't need those forward declarations. This is completely up to you, of course. I am just trying to give (or remind) more context to make a decision keeping in mind different aspects.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141779