[PATCH] D59013: [CMake][runtimes] Move libunwind, libc++abi and libc++ to lib/ and include/

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