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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.
Herald added a subscriber: mikhail.ramalho.

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

> Thanks for the stack of patches! This is a really nice start. Before we commit to going into a specific direction, I think it is useful to lay out how we envision things being at the end, and then roughly how we're going to get there.
>
> Here's a starting point. At the end, I think what we want to have is (this doesn't necessarily reflect *how* we're going to get there):
>
> 1. No more `<__pstl_algorithm>` & other glue headers -- instead the PSTL versions would be defined in suitably named `<__algorithm/pstl_any_of.h>` headers and included directly inside `<algorithm>`.
> 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.

+1 for as little divergence from upstream as possible.

> 5. The configuration headers `__pstl_config_site.in`, `pstl_config.h` can be simplified to only contain the macros we actually need (pruning platforms and compilers we don't support), and then folded into `__config_site` and `__config` as appropriate.
> 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>.
> 7. We should have our own tests for these algorithms and they should follow the usual libc++ testing conventions. I think we should base them on the existing pstl tests but we can add them as we iterate on steps (1-2-3).
>
> In terms of how to get there, I think your current approach makes sense, however I would start with a clean slate. So before this patch, I would create a patch that basically removes the current integration (including the `pstl` test symlink) so that we start the new integration from a clean slate.
>
> @philnik I think this properly describes what you had planned on doing (and which I agree is the right thing to do). Additional thoughts/comments?
>
> Also CCing a few folks for awareness since they might have opinions and it would be good to get a common agreement on the direction before we start chipping away at the algorithms. @rarutyun @Mordante @var-const

I'm not too familiar with pstl so I don't have a strong opinion. What I read sounds sensible, thanks @philnik &
@ldionne for the detailed plan. I think it would be great to put some of these design ideas in a developer page for our documentation. There we can also link to the upstream projects we provide.

Not an issue for this patch series, but I wonder how we can properly sync with upstream. We've inherited the ryu algorithm for `to_chars` but we don't sync up with upstream MSVC STL or Ulf's GitHub.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141779