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

Reply via email to