[PATCH] D15784: Enable 2 warnings on MSVC, turn on StringPooling & intrinsic functions
Alexander Riccio via llvm-commits
llvm-commits at lists.llvm.org
Sun Dec 27 15:17:00 PST 2015
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)
> 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)
> 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.
More information about the llvm-commits