[PATCH] D60480: [libc++] Integrate the PSTL into libc++

[Louis Dionne via Phabricator via llvm-commits]

ldionne marked an inline comment as done.
ldionne added inline comments.


================
Comment at: libcxx/lib/CMakeLists.txt:125
 
+find_package(ParallelSTL)
+if (NOT TARGET pstl::ParallelSTL)
----------------
MikeDvorskiy wrote:
> ldionne wrote:
> > MikeDvorskiy wrote:
> > > ldionne wrote:
> > > > The basic idea is that when `ParallelSTL` is installed on the system (or if it's in the tree), we just find it with `find_package` and then declare a dependency on it and everything should work.
> > > > ParallelSTL is installed on the system
> > > 
> > > What does it mean? The fact there is no ParallelSTL as "product/component". ParallelSTL - just a set of headers.
> > > 
> > > So, the cmake file should find "ParallelSTL package".
> > > 
> > Either it's in the tree and we can find it (like we do here), or it's somewhere on the system and has a `ParallelSTLConfig.cmake` file installed alongside it (which our current pstl build does install). Realistically, I only expect the former to be used.
> Parallel STL is not a separate product/package, so it doesn't make sense to have formal versioning for it. Also it doesn't make sense  installation part from CMakeLists.txt.
> 
> We won't be able to use find_package(ParallelSTL) (and find_package(ParallelSTL <version>) as well) in order to include Parallel STL into user project. But it is possible to use add_subdirectory(/path/to/parallelstl).
> 
> WDYT?
> Parallel STL is not a separate product/package, so it doesn't make sense to have formal versioning for it. Also it doesn't make sense installation part from CMakeLists.txt.

It definitely is a separate package, no? It's a self-standing library that libc++ happens to be using as an implementation detail. I can't see another way to look at this other than saying it's a bunch of sources that we copy-paste into libc++. But that way of looking at things isn't good because it means it doesn't make sense to build/test the PSTL on its own, which we want to do.


> We won't be able to use find_package(ParallelSTL) 

It does work if the PSTL is installed (which installs a `ParallelSTLConfig.cmake` file) before building libc++. Otherwise, it also works if the ParallelSTL is enabled as part of the monorepo, because even though the call to `find_package(ParallelSTL)` will fail, the `pstl::ParallelSTL` CMake target still exists.

Also, we're commenting on an old diff.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60480