Ericson2314 added inline comments.
================ Comment at: libcxx/CMakeLists.txt:421 +if(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR AND NOT APPLE) + set(default_install_path "${CMAKE_INSTALL_INCLUDEDIR}/${LLVM_DEFAULT_TARGET_TRIPLE}/c++/v1") +else() ---------------- ldionne wrote: > ldionne wrote: > > Instead, I'd do this: > > > > ``` > > if(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR AND NOT APPLE) > > set(_include_target_dir > > "${CMAKE_INSTALL_INCLUDEDIR}/${LLVM_DEFAULT_TARGET_TRIPLE}/c++/v1") > > set(_install_library_dir > > "lib${LLVM_LIBDIR_SUFFIX}/${LLVM_DEFAULT_TARGET_TRIPLE}") > > else() > > set(_include_target_dir "${LIBCXX_INSTALL_INCLUDE_DIR}") > > set(_install_library_dir "lib${LLVM_LIBDIR_SUFFIX}") > > endif() > > > > set(LIBCXX_INSTALL_INCLUDE_TARGET_DIR "${_include_target_dir}" CACHE PATH > > "Path where target-specific libc++ headers should be installed.") > > set(LIBCXX_INSTALL_LIBRARY_DIR "${_install_library_dir}" CACHE PATH > > "Path where built libc++ libraries should be installed.") > > ``` > > > > IMO that's easier on the eye. > Gentle ping on this. Ah right, I have this "throw away temporary style as soon as possible" on all the runtimes right now. Would you prefer I do instead do "define all the defaults, define all the cache vars" for all of them? I am a little partial to the current style because if it is closer to what I would do if cmake has proper expressions. (something like: ``` set(LIBCXX_INSTALL_INCLUDE_TARGET_DIR "${ if(...) stuff else() stuff endif() }" CACHE PATH "Path where target-specific libc++ headers should be ``` ). But if that if you are sure your want it consistently the other way I can switch it. A bigger issue might be when the variables depend on each other, e.g. when I get the library install dir base name for the library binary dir. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132608/new/ https://reviews.llvm.org/D132608 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits