[PATCH] [CMake] Don't pass in MSVC warning flags as definitions

[Reid Kleckner]

lgtm


================
Comment at: cmake/modules/HandleLLVMOptions.cmake:296
@@ +295,3 @@
+
+  foreach(flag ${msvc_warning_flags})
+    append("${flag}" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
----------------
rnk wrote:
> It might also be faster to string-ify the list like this:
>   string(REPLACE ";" " " msvc_warning_flags "${msvc_warning_flags}")
> 
> And append that one string to every CMAKE_*_FLAGS var we need to touch.
No need to implement this, given that we are only appending twice.

================
Comment at: cmake/modules/HandleLLVMOptions.cmake:297
@@ +296,3 @@
+  foreach(flag ${msvc_warning_flags})
+    append("${flag}" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
+  endforeach(flag)
----------------
gbedwell wrote:
> gbedwell wrote:
> > rnk wrote:
> > > Does this actually work in the VS generator? I think you need to tweak CMAKE_C_DEBUG_FLAGS, CMAKE_C_RELEASE_FLAGS, etc. You can search around for instances of for loops over CMAKE_BUILD_TYPES to see how this is done.
> > With the VS2013 generator 'Visual Studio 12 Win64' and CMake 3.1.3 I can see these flags in the solution properties for all build configurations so it seems to work on my setup.  I'll test on CMake 2.8.12.2 on the plain 'Visual Studio 12' generator too just to be sure.  If these tests show it to work, does this implementation seem reasonable?  Are there other reasons why it is preferable for them to appear in the individual build config flag lists?
> I've now verified that with this patch the expected warnings are also disabled/promoted for all build configs with CMake 2.8.12.2 and the plain Visual Studio 12 generator.  I'm happy to change it, but I don't think it's required unless there's another reason I've not not considered.
Nope, this seems good to me.

http://reviews.llvm.org/D8188

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/