[Lldb-commits] [PATCH] D61956: [CMake] Add first CMake cache files

[Alex Langford via Phabricator via lldb-commits]

xiaobai added inline comments.


================
Comment at: lldb/cmake/caches/Apple-lldb-macOS.cmake:18
+set(CMAKE_BUILD_TYPE RelWithDebInfo)
+set(LLVM_ENABLE_MODULES ON CACHE BOOL "")
----------------
xiaobai wrote:
> labath wrote:
> > sgraenitz wrote:
> > > compnerd wrote:
> > > > sgraenitz wrote:
> > > > > Can / Should we add this? Here?
> > > > This is fine to add assuming that you are building on Darwin since that implies clang and that will work.  I would say that if you really want to be pedantically correct, it would be better to do:
> > > > 
> > > > ```
> > > > if(CMAKE_CXX_COMPILER_ID MATCHES Clang)
> > > >   set(LLVM_ENABLE_MODULES_ON CACHE BOOL "")
> > > > endif()
> > > > ```
> > > > 
> > > > Note that the `MATCHES` is required here to match both `Clang` and `AppleClang`.
> > > That sounds reasonable. I would take your version and put it in the base cache?
> > > 
> > > The other question was, whether `RelWithDebInfo` is a good default. Personally, I use it far more often than other configurations. (Running the test suite with a debug Clang is just too slow.) Moving to the base cache too.
> > Are you sure that `CMAKE_CXX_COMPILER_ID` is available this early in the initialization ?
> I think RelWithDebInfo is an okay default. I personally don't like to set build types in caches because I think it's reasonable to expect the user to specify their build type, but if you mostly use that one build type then it's fine.
I don't think `CMAKE_CXX_COMPILER_ID` is available at the cache processing stage of initialization, so this is probably not something you can do. Maybe you can guard with the condition that the OS is Darwin? Maybe that's not needed, since the cache is Apple-specific anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61956