[libc-commits] [PATCH] D132608: [CMake] Clean up CMake binary dir handling

[John Ericson via Phabricator via libc-commits]

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