[PATCH] D116709: [CMake][WinMsvc] Fix user passed compiler/linker flags

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added inline comments.


================
Comment at: llvm/cmake/platforms/WinMsvc.cmake:253-257
+list(APPEND _CTF_NATIVE_DEFAULT "-DCMAKE_C_FLAGS=\"\"")
+list(APPEND _CTF_NATIVE_DEFAULT "-DCMAKE_CXX_FLAGS=\"\"")
+list(APPEND _CTF_NATIVE_DEFAULT "-DCMAKE_EXE_LINKER_FLAGS=\"\"")
+list(APPEND _CTF_NATIVE_DEFAULT "-DCMAKE_MODULE_LINKER_FLAGS=\"\"")
+list(APPEND _CTF_NATIVE_DEFAULT "-DCMAKE_SHARED_LINKER_FLAGS=\"\"")
----------------
I'm not a fan of this. A pattern I like to follow is to pass a `CMAKE_TOOLCHAIN_FILE` to `CROSS_TOOLCHAIN_FLAGS_NATIVE`, so that the native build flags get configured via a toolchain file instead of ad-hoc variable setting. Pre-set cached copies of these variables will interfere with that toolchain file's ability to set flags, and in particular, it'll break the toolchain file's use of the *_INIT variables, which is kinda ironic given the purpose of this diff :)

Would unsetting the env vars in the toolchain file instead accomplish the same goal?


================
Comment at: llvm/cmake/platforms/WinMsvc.cmake:292-293
 # afterward.
-set(_CMAKE_C_FLAGS_INITIAL "${CMAKE_C_FLAGS}" CACHE STRING "")
-set(CMAKE_C_FLAGS "${_CMAKE_C_FLAGS_INITIAL} ${COMPILE_FLAGS}" CACHE STRING "" FORCE)
+set(_CMAKE_C_FLAGS_INITIAL "${CMAKE_C_FLAGS_INIT}" CACHE STRING "")
+set(CMAKE_C_FLAGS_INIT "${_CMAKE_C_FLAGS_INITIAL} ${COMPILE_FLAGS}" CACHE STRING "" FORCE)
 
----------------
I think we can avoid having the two separate variables, and just do a direct setting, if we make sure this toolchain file is only evaluated once per CMake invocation. Now that we've raised our minimum CMake version, just adding

```
include_guard(GLOBAL)
```

at the top ought to do the trick.

It's fine to leave that for a follow-up change (or not do it at all).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116709