[Lldb-commits] [PATCH] D56440: [CMake] Phase out LLDB_TEST_C/CXX_COMPILER in favor of single LLDB_TEST_COMPILER

[Stella Stamenova via Phabricator via lldb-commits]

stella.stamenova added inline comments.


================
Comment at: CMakeLists.txt:61
+  # Use LLDB_TEST_COMPILER_IS_DEFAULT to determine whether or not to replace
+  # CMAKE_CFG_INTDIR with LLVM_BUILD_MODE for dotest.
+  if(LLDB_TEST_COMPILER)
----------------
If it's true that we don't need the CMAKE_CFG_INTDIR replacement in the test cmake file, then is this comment still correct?


================
Comment at: lit/CMakeLists.txt:14
 
-if (NOT LLDB_TEST_USE_CUSTOM_C_COMPILER)
-  string(REPLACE ${CMAKE_CFG_INTDIR} ${LLVM_BUILD_MODE} LLDB_TEST_C_COMPILER "${LLDB_TEST_C_COMPILER}")
----------------
sgraenitz wrote:
> stella.stamenova wrote:
> > Your change is removing the logic to setup this path correctly. This will cause failures in the tests.
> That should be fine. It only changed the value of `LLDB_TEST_C/CXX_COMPILER` in the scope of this CMakeLists.txt/directory. The value in the parent CMakeLists.txt as well as the one in the cache remained unchanged.
> 
> Thus it is unused since https://reviews.llvm.org/rC347216#change-H2HV4zA8ol05 removed their usage from `lit/lit.site.cfg.py.in` that gets configured below. Before this change the local value was passed to the config file like this:
> ```
> config.cc = "@LLDB_TEST_C_COMPILER@"
> config.cxx = "@LLDB_TEST_CXX_COMPILER@"
> ```
> 
> They seem to be inferred via `LLDB_LIT_TOOLS_DIR` now.
Have you tested it with a tool set that supports multiple configurations to make sure that is the case? I believe XCode and VS both support multiple configurations.


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

https://reviews.llvm.org/D56440