[PATCH] D59013: [CMake][runtimes] Move libunwind, libc++abi and libc++ to lib/ and include/
Shoaib Meenai via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Mar 6 20:43:18 PST 2019
smeenai added inline comments.
================
Comment at: clang/test/Driver/linux-per-target-runtime-dir.c:13
// CHECK-PER-TARGET-RUNTIME: "-isysroot" "[[SYSROOT:[^"]+]]"
// CHECK-PER-TARGET-RUNTIME: "-internal-isystem" "[[RESDIR]]/include/c++/v1"
// CHECK-PER-TARGET-RUNTIME: "-internal-isystem" "[[SYSROOT]]/usr/local/include"
----------------
Shouldn't this one be different now?
================
Comment at: libcxx/CMakeLists.txt:421
+ set(DEFAULT_INSTALL_PREFIX lib${LLVM_LIBDIR_SUFFIX}/${LLVM_DEFAULT_TARGET_TRIPLE}/)
+ set(LIBCXX_LIBRARY_DIR ${LLVM_LIBRARY_OUTPUT_INTDIR}/${LLVM_DEFAULT_TARGET_TRIPLE}${LIBCXX_LIBDIR_SUBDIR})
+ set(LIBCXX_HEADER_DIR ${LLVM_BINARY_DIR})
----------------
It's expected for LIBCXX_LIBDIR_SUBDIR to always begin with a `/`, correct? I feel like it would be less error-prone to have places using that variable add the `/` rather than relying on the variable containing it. In particular, since the variable name contains SUBDIR, I wouldn't expect to have to specify the leading `/` of the subdirectory myself.
Case in point: if I'm reading correctly, `DEFAULT_INSTALL_PREFIX` will end with a trailing `/`, and then `LIBCXX_INSTALL_LIBDIR` will begin with a leading `/`, so you'll end up with two consecutive `/` when you join the two in lib/CMakeLists.txt. Not a big deal, but also easily avoidable, I think.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D59013/new/
https://reviews.llvm.org/D59013
More information about the llvm-commits
mailing list