Do not set CMAKE_CXX_FLAGS_<config> with FORCE

[ArtemSkrebkov]

Summary

Updating CMAKE_CXX_FLAGS_RELEASE (with CACHE FORCE) forces
to re-initialize it each time with a new value taken from cache.

In my project , it leads to CMAKE_CXX_FLAGS_RELEASE like this /MD /O2
/Ob2 /DNDEBUG /sdl /guard:cf /wd4996 /wd4146 /wd4703 /wd4996 /wd4146
/wd4703 /wd4996 /wd4146 /wd4703 /wd4996 /wd4146 /wd4703 repeating flags
again.

The consequences of this is that the project has to be rebuilt from
scratch every time you call cmake configure.

[ArtemSkrebkov]

@aaronpuchert @beanz

I added you as reviewers since you had touched this file last year :)

[aaronpuchert]

I did a minor cleanup that touched this file, but I don't actually use MSVC. I think @beanz is the right reviewer, just a very busy one. ;)

FWIW, it seems reasonable to me, especially compared with line 41. If we can't get @beanz to look at it, I can also accept it.

[beanz]

Sorry, this slipped my notice. @mstorsjo made some changes in this area recently with D155233. The approach there is to migrate to using the CMake built-in option CMAKE_MSVC_RUNTIME_LIBRARY.

@ArtemSkrebkov, does the issue you were encountering still occur on main now or did D155233 resolve it?

[ArtemSkrebkov]

FORCE setting is in place after https://reviews.llvm.org/D155233 , so I guess it still should be the case.

But let me check to be sure.

[beanz]

In D151613#4603055, @ArtemSkrebkov wrote:

FORCE setting is in place after https://reviews.llvm.org/D155233 , so I guess it still should be the case.

But let me check to be sure.

The problem isn’t actually with the FORCE flag. The problem you were seeing was caused by CMake and LLVM’s configure scripts each having its own way of setting those flags. When these scripts were first written CMake didn’t have an official way to set those flags. LLVM adopts new CMake behaviors fairly slowly to maintain support for older versions of CMake.

The code today removes the -M* options from the C and CXX flags variables and relies on setting the CMAKE_MSVC_RUNTIME_LIBRARY variable instead. @mstorsjo could speak more about the need for it to be FORCEd, but my guess is that since that is a CMake option, a default value for it gets set, and _only_ when translating the LLVM option to the CMake option we need to FORCE overwriting the value. So if you don’t pass LLVM_USE_CRT_*, the FORCE line shouldn’t get hit.

[mstorsjo]

In D151613#4603480, @beanz wrote:

In D151613#4603055, @ArtemSkrebkov wrote:

FORCE setting is in place after https://reviews.llvm.org/D155233 , so I guess it still should be the case.

But let me check to be sure.

The problem isn’t actually with the FORCE flag. The problem you were seeing was caused by CMake and LLVM’s configure scripts each having its own way of setting those flags. When these scripts were first written CMake didn’t have an official way to set those flags. LLVM adopts new CMake behaviors fairly slowly to maintain support for older versions of CMake.

The code today removes the -M* options from the C and CXX flags variables and relies on setting the CMAKE_MSVC_RUNTIME_LIBRARY variable instead. @mstorsjo could speak more about the need for it to be FORCEd, but my guess is that since that is a CMake option, a default value for it gets set, and _only_ when translating the LLVM option to the CMake option we need to FORCE overwriting the value. So if you don’t pass LLVM_USE_CRT_*, the FORCE line shouldn’t get hit.

I don't have much insight/opinion on the use of FORCE here, I just refactored this piece of code to keep it around somewhat, until everybody are transitioned over to CMAKE_MSVC_RUNTIME_LIBRARY.

[ArtemSkrebkov]

@beanz you are right :) I confirm that my issue is not reproducible anymore. Thanks for sharing the insights on CMake handling in LLVM.

llvm/cmake/modules/ChooseMSVCCRT.cmake

 macro(set_flag_in_var flagsvar regex flag)
   string(REGEX MATCH "${${regex}}" current_flag "${${flagsvar}}")
   if("${current_flag}" STREQUAL "")
     set(${flagsvar} "${${flagsvar}}${${flag}}")
   else()
     string(REGEX REPLACE "${${regex}}" "${${flag}}" ${flagsvar} "${${flagsvar}}")
   endif()
   string(STRIP "${${flagsvar}}" ${flagsvar})
-  # Make sure this change gets reflected in the cache/gui.
-  # CMake requires the docstring parameter whenever set() touches the cache,
-  # so get the existing docstring and re-use that.
-  get_property(flagsvar_docs CACHE ${flagsvar} PROPERTY HELPSTRING)
-  set(${flagsvar} "${${flagsvar}}" CACHE STRING "${flagsvar_docs}" FORCE)
+  set(${flagsvar} "${${flagsvar}}")
 endmacro(set_flag_in_var)
 
 
 macro(choose_msvc_crt MSVC_CRT)
   if(LLVM_USE_CRT)
     message(FATAL_ERROR
       "LLVM_USE_CRT is deprecated. Use the CMAKE_BUILD_TYPE-specific
 variables (LLVM_USE_CRT_DEBUG, etc) instead.")