[PATCH] D15784: Enable 2 warnings on MSVC, turn on StringPooling & intrinsic functions

[Alexander Riccio via llvm-commits]

ariccio marked 2 inline comments as done.
ariccio added a comment.

> I'm certainly the wrong person to review this


Ooops. I just saw CMake, and assumed so. Sorry!

Otherwise: comments addressed? I think I need to submit this so that my replies are submitted?


================
Comment at: cmake/modules/HandleLLVMOptions.cmake:369
@@ +368,3 @@
+  # Eliminate Duplicate Strings
+  append("/GF" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
+
----------------
aaron.ballman wrote:
> I think this one is reasonable, but I worry about it requiring use of /bigobj. What are the size differences for our executables with this option disabled vs enabled?
> I think this one is reasonable, but I worry about it requiring use of /bigobj.

MSVC didn't complain about it in the debug build, so my guess is that this isn't a problem, but I'll compile a release build - optimizations might somehow pack more strings into a single image? - and see if it causes problems there. Can you think of any situation that might actually include more than 65,536 strings?

> What are the size differences for our executables with this option disabled vs enabled?

Ok, I'm embarrassed to say it, but I don't have any data for this. LLVM just takes too long to compile for me to check the executable size for such a small patch. Is that reasonable, or should I actually compare them?

================
Comment at: cmake/modules/HandleLLVMOptions.cmake:375
@@ +374,3 @@
+  # Enforce type conversion rules
+  append("/Zc:rvalueCast" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
+
----------------
aaron.ballman wrote:
> Should this be specified for CMAKE_C_FLAGS as well? I know MSVC doesn't really distinguish between the C and C++ compiler, but it seems a bit strange (though likely harmless).
Whoops - I think you're right. I'm still quite new to CMake, and I instinctively copied the other "append" statements. MSDN doesn't mention any issues when passing rvalueCast while compiling as C, but it's probably safer to only pass while compiling C++.



Sidenote: the same should apply to /Zc:sizedDealloc- (which I didn't add)? [[ https://twitter.com/StephanTLavavej/status/681246530074849280 | MSDN doesn't actually document it ]], so I can't say much about it.



http://reviews.llvm.org/D15784