phosek added inline comments.
================
Comment at: libcxx/lib/CMakeLists.txt:269
+ AND (TARGET cxxabi_static OR HAVE_LIBCXXABI))
+ #if ((TARGET ${LIBCXX_CXX_ABI_LIBRARY}) OR
+ #(${LIBCXX_CXX_ABI_LIBRARY} MATCHES "cxxabi(_static|_shared)?" AND
HAVE_LIBCXXABI))
----------------
ldionne wrote:
> phosek wrote:
> > ldionne wrote:
> > > phosek wrote:
> > > > ldionne wrote:
> > > > > I don't understand why any of this needs to change -- can you please
> > > > > explain? Also, you probably didn't mean to leave the commented-out
> > > > > lines.
> > > > The reason this change is needed the case when we're linking shared
> > > > libc++abi into shared libc++ in which case `${LIBCXX_CXX_ABI_LIBRARY}`
> > > > will be set to `cxxabi_shared` in `HandleLibCXXABI.cmake` but we cannot
> > > > merge `libc++abi.so` into `libc++.a`, so instead we force the use of
> > > > `cxxabi_static` here.
> > > >
> > > > Alternatively, we could modify `HandleLibCXXABI.cmake` to set two
> > > > dependencies, one for the static case and one for the shared case and
> > > > use the former one here.
> > > >
> > > > Removed the commented out lines.
> > > Thanks. There's something I still don't understand. If you are linking
> > > the ABI library dynamically, why would you want to merge it (well, the
> > > static version of it) into `libc++.a`? It seems like this is somewhat
> > > defeating the purpose of dynamically linking against the ABI library, no?
> > In our case, we want to link shared library dynamically and merge all
> > static libraries, so `LIBCXX_STATICALLY_LINK_ABI_IN_SHARED_LIBRARY=OFF` and
> > `LIBCXX_STATICALLY_LINK_ABI_IN_STATIC_LIBRARY=ON`. This means that
> > `LIBCXX_CXX_ABI_LIBRARY` will be set to `cxxabi_shared` and it's what will
> > be used other places throughout this file as the interface library.
> > However, we cannot merge `libc++abi.so` into `libc++.a` so that's why we
> > have to explicitly select `cxxabi_static` here rather than simply using
> > `LIBCXX_CXX_ABI_LIBRARY` as before.
> >
> > Like I mentioned in the previous comment, alternative would be to split
> > this into two variables, e.g. `LIBCXX_CXX_SHARED_ABI_LIBRARY` and
> > `LIBCXX_CXX_STATIC_ABI_LIBRARY`, would you prefer that approach?
> Yes, I understand why the `TARGET_LINKER_FILE` must also be `cxxabi_static`.
> What I don't understand is why the conditions need to change, i.e. why you're
> going from
> ```
> if (TARGET ${LIBCXX_CXX_ABI_LIBRARY} OR
> (${LIBCXX_CXX_ABI_LIBRARY} MATCHES "cxxabi(_static|_shared)?" AND
> HAVE_LIBCXXABI))
> ```
>
> to
>
> ```
> if (${LIBCXX_CXX_ABI_LIBRARY} MATCHES "cxxabi(_static|_shared)?"
> AND (TARGET cxxabi_static OR HAVE_LIBCXXABI))
> ```
>
> Note that I do think your new condition make more sense (since if the ABI
> library is not a target, we should fall back on the `else()` branch). I just
> want to understand why you're changing it.
>
> > Like I mentioned in the previous comment, alternative would be to split
> > this into two variables, e.g. `LIBCXX_CXX_SHARED_ABI_LIBRARY` and
> > `LIBCXX_CXX_STATIC_ABI_LIBRARY`, would you prefer that approach?
>
> I don't think that is necessary at this point.
>
You're right, that's a leftover from previous iterations.
Repository:
rL LLVM
https://reviews.llvm.org/D49502
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
