[libcxx-commits] [PATCH] D60480: [WIP] integration of pstl into libc++

Wed Apr 10 08:33:03 PDT 2019

rodgert added a comment.

In D60480#1461369 <https://reviews.llvm.org/D60480#1461369>, @ldionne wrote:

> In D60480#1460728 <https://reviews.llvm.org/D60480#1460728>, @rodgert wrote:
>
> > In D60480#1460300 <https://reviews.llvm.org/D60480#1460300>, @ldionne wrote:
> >
> > > The purpose of this review is to expose how I plan to integrate the parallel STL into libc++, and to discuss some changes that are desirable in pstl for this to be possible. This is a work in progress, obviously, and it's meant as a concrete support for having discussions.
> > >
> > > Concrete changes I think we need to make in pstl:
> > >
> > > - Rename all the files to double underscores.
> >
> >
> > This isn't a libstdc++ convention, but I'm not opposed.
>
>
> The problem with not doing that is that our headers could collide with user's header files.

I understand the complication. libstdc++ follows the convention of everything 'internal' being in bits/ and then the std headers #include <bits/foo.h>, and this is something that's 
on my list to address for PSTL (as what is currently in libstdc++ could still clash with user headers). I don't have any real objection to the underscore naming convention. I can mechanically
translate them to #include <pstl/non_underscore_named_file.h>.

>> 
>> 
>>> - Move everything that's meant to be included directly by standard library implementations out of `internal/`, since that doesn't make sense. It's not internal, it's the API of PSTL (which is meant to be consumed by standard libraries).
>> 
>> I would like to maintain a clear separation between the bits that implement PSTL and the glue bits that tie into a given library. Currently everything in 'internal/' goes to libstdc++v3/include/pstl when I integrate with libstdc++v3. I don't want to be left guessing what those files are, I want them to move as a cohesive unit into the library's implementation detail. We can name whatever we want.
> 
> Right, I agree. What I meant (but that's not what I wrote) is that the glue headers and everything that is expected to be used from standard libraries should be out of `internal`. The backend and the config stuff can (and should) stay in `internal`. Do we agree here?

Ok, yeah, I'm good with that.

>> 
>> 
>>> - We could provide the glue definitions and declarations as `.ipp` files that are meant to be included by standard libraries as-is. The Standard library could then define stuff like the namespace in which those declarations are injected, etc.
>> 
>> I would be interested in exploring this, subject to the caveat that whole of what's currently in 'internal' is an obvious package of related implementation detail that I can poke into libstdc++ as a unit.
> 
> Well, so only the glue part would be `.ipp`s. And I don't care if they're headers or `.ipp`s, the idea is just that I want to reverse the model from "libc++ includes pstl headers that define the parallel algorithms" to "pstl provides declarations that a standard library can copy-paste into their own headers via `#include`".

I'm also good with this.

>>> I also suggest:
>>> 
>>> - Removing the `<algorithm>` & friends headers from PSTL itself. Those don't belong there, they belong with the standard library.
>>> 
>>>   WDYT @rodgert ?
>> 
>> If we remove them, then we sort of lose the ability to test pstl outside of standard library, no? I'm not going to be testing or packaging libc++ most likely, but I am going to be contributing changes here and running tests within the context of libstdc++. Having the ability to test changes to the library independent of any integration with another standard library is still useful for me.
> 
> I believe those headers should be provided by a dummy standard library that we use only to run the tests. Basically we'd have a dummy standard library in `pstl/test` that would define these headers, and that's what we would run the tests against.

That would work for me...AND...we can probably remove the per-test #ifdef for the standalone checks, centralize it in the dummy standard headers in 'pstl/test' which would be much cleaner than what is currently there.

> Lastly, about the `__PSTL_XXX` vs `_PSTL_XXX`: This patch is using what's in PSTL today, that's all. As a separate change, I thought about changing all `__PSTL_XXX` macros to be `_PSTL_XXX` to follow the more common convention (underscore-capital or double-underscore-lowercase), but this is a change separate from the integration into libc++.

JF and I discussed possibly some tooling for this sort of thing. I'm still interested in looking at that, I just don't see a lot of value in changing it by hand just to comply with conflicting conventions between standard library implementations.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60480