[libcxx-commits] [PATCH] D103198: [libc++] Add a CI configuration where we test the PSTL integration

[Thomas Rodgers via libcxx-commits]

Circling back on this -

I don't disagree with any of Louis' points. Regarding circular header
dependencies, I would also like to switch the PSTL to use libstdc++'s
internal implementation headers. I am not really sure how to do that and
still incorporate upstream changes going forward.

I expect to start rebasing libstdc++'s implementation on the current
version of the upstream next month.

On Fri, Jan 13, 2023 at 9:22 AM Louis Dionne via Phabricator <
reviews at reviews.llvm.org> wrote:

> ldionne requested changes to this revision.
> ldionne added a comment.
> This revision now requires changes to proceed.
>
> I just had a discussion with @philnik about this patch. I have come to
> think that attempting to use the shared version of pstl inside libc++ was
> not a good idea to begin with.
>
> Standard libraries have subtly different requirements and ways of doing
> things. For example, we use an ABI tag on our implementation-detail
> functions so that we have the freedom to change them however we please from
> release to release. Last I checked, I think libstdc++ handles this
> differently by not making pre/post condition changes even in
> implementation-detail functions. Those are both valid ways of doing things
> and I am not criticizing, however trying to share *header* code between the
> two standard libraries will surface these kinds of fundamental differences,
> some of which can't be reconciled with any amount of macros. Another
> example is circular header dependencies -- without being able to include
> implementation-detail headers of libc++ from the pstl, we may not be able
> to get rid of some circular dependencies, and we certainly won't be able to
> achieve the same level of granularity that we have in libc++. Yet another
> example is how we deal with removing incidental transitive includes -- we
> have a process for doing it, but it's not clear to me how we'd establish
> this in a shared pstl library.
>
> In other words, I think that sharing header-level APIs between standard
> libraries is not a good idea. Sharing implementation that lives in a dylib
> or static archive is 100% fine since we can treat it as a third-party
> dependency. Sharing a well-defined backend implementation is also probably
> fine, but sharing headers is a different can of worms.
>
> My opinion about this is even stronger after seeing what the decision of
> using an external code-base in our headers has given us so far -- not much.
> Because of the complexity of integrating pstl properly into libc++, we've
> basically been sitting on this and a few other difficult-ish questions ever
> since it was contributed, leading to us not shipping anything for years.
> Instead, if we had this code directly in libc++, I am confident we would
> have shipped *something* at this point and started making improvements to
> it.
>
> @philnik I would suggest we abandon this revision and instead integrate
> the pstl directly into libc++, marking it as an experimental feature per
> our usual conventions. We should strive not to touch the backends too much
> so as to make it easy (but not necessarily automatic) to keep that in sync
> with the actual pstl code. That's where 90% of changes happen anyway. Rough
> suggested path:
>
> - Copy over as-is to libc++
> - Add a CI job that enables `_LIBCPP_HAS_PARALLEL_ALGORITHMS` -- no big
> CMake changes needed
> - Clang-format the public API, but don't touch the back-ends since we're
> going to try to share that
> - Apply libc++ macros as needed (e.g. `_LIBCPP_HIDE_FROM_ABI`)
> - Use the right namespaces
> - Get rid of `pstl_config.h` in favour of `<__config>`
> - Granularize public API headers
> - Now we should be able to simplify the glue like `<__pstl_algorithm>`
> inclusion to instead include our own granularized headers from `<algorithm>`
> - We should be able to drop the custom `_LIBCPP_HAS_PARALLEL_ALGORITHMS`
> and use our existing machinery for experimental features
> - We should also import the tests, since right now we're not testing
> anything
>
> Doing that, we should strive to maintain the clear boundary between the
> backends and the public API. The public API is ours and we can change it --
> the backend is where the real "work" is, and we want to share that with the
> pstl as much as possible since folks are making updates and improvements to
> it.
>
> @rodgert This repo is not going to go anywhere, so this will not have an
> impact on your ability to keep taking updates made to pstl. We (libc++)
> will just start porting changes from pstl into libc++ manually when they
> happen (which isn't *that* often so far).
>
>
> Repository:
>   rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D103198/new/
>
> https://reviews.llvm.org/D103198
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/libcxx-commits/attachments/20230303/fe7b66ca/attachment.html>