[libcxx-commits] [PATCH] D137337: Replace LLVM_LIBDIR_SUFFIX by CMAKE_INSTALL_LIBDIR

[Michał Górny via Phabricator via libcxx-commits]

mgorny added inline comments.


================
Comment at: bolt/lib/RuntimeLibs/RuntimeLibrary.cpp:32
   SmallString<128> LibPath = llvm::sys::path::parent_path(Dir);
-  llvm::sys::path::append(LibPath, "lib" LLVM_LIBDIR_SUFFIX);
+  llvm::sys::path::append(LibPath, CMAKE_INSTALL_LIBDIR);
   if (!llvm::sys::fs::exists(LibPath)) {
----------------
Well, one thing I immediately notice is that technically `CMAKE_INSTALL_LIBDIR` can be an absolute path, while in many locations you are assuming it will always be relative. Not saying that's a blocker but I think you should add checks for that into `CMakeLists.txt`.


================
Comment at: clang/tools/libclang/CMakeLists.txt:234
           DESTINATION
-            "lib${LLVM_LIBDIR_SUFFIX}/python${PythonVersion}/site-packages")
+          "${CMAKE_INSTALL_LIBDIR}/python${PythonVersion}/site-packages")
 endforeach()
----------------
You seem to be changing indentation here, and below.

On unrelated note, hardcoding site-packages path sounds like a bad idea but that's a problem for another patch.


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

https://reviews.llvm.org/D137337