[PATCH] Disable specific MSVC warnings so that W4 warnings can be enabled.

[David Blaikie]

REPOSITORY
  rL LLVM

================
Comment at: cmake/modules/HandleLLVMOptions.cmake:286
@@ +285,3 @@
+    -wd4706 # Suppress 'assignment within conditional expression'
+    -wd4310 # Suppress 'cast truncates constant value'
+    -wd4701 # Suppress 'potentially uninitialized local variable'
----------------
You mentioned this one fired ~200 times - this might actually be worth doing a small sample check to see what it's firing on. Perhaps these are things we should fix - unless they appear in dependent expressions in templates, in which case they're probably false positives.

The rest of these disables I'm totally fine with - if you want, please commit them separately/ahead of the rest of the review.

================
Comment at: lib/Support/APFloat.cpp:1433
@@ -1432,3 +1432,3 @@
      an addition or subtraction.  */
-  subtract ^= sign ^ rhs.sign;
+  subtract ^= (sign ^ rhs.sign) != 0;
 
----------------
andrew.w.kaylor wrote:
> This addresses warning C4805: unsafe mix of type <X> and type <Y> in operation.
This one's been fixed (slightly differently) by Aaron Ballman. We had some more discussions about the same issues/principles in that review thread too - I think he's OK with the idea of disabling this warning.

================
Comment at: lib/Target/ARM/InstPrinter/ARMInstPrinter.cpp:641
@@ -640,3 +640,3 @@
   unsigned Op = ARM_AM::getAM5Op(MO2.getImm());
-  if (AlwaysPrintImm0 || ImmOffs || Op == ARM_AM::sub) {
+  if (ImmOffs || Op == ARM_AM::sub || AlwaysPrintImm0) {
     O << ", "
----------------
andrew.w.kaylor wrote:
> This addresses warning C4189: local variable is initialized but not referenced.
> 
> AlwaysPrintImm0 is a template parameter that can short circuit the condition so that ImmOffs and Op are never referenced.
This seems like a step backwards, actually - we try to deliberately put the cheap tests first (and a compile time constant is the cheapest test). Granted any compiler worth its salt will kill the dead expressions, but there's no need to ask it to do that work.

I'm fairly strongly in favor of disabling this warning rather than making this code change.

================
Comment at: lib/Target/NVPTX/NVPTXAsmPrinter.cpp:507
@@ -506,4 +506,3 @@
   unsigned RegNo = MI->getOperand(0).getReg();
-  const TargetRegisterInfo *TRI = nvptxSubtarget->getRegisterInfo();
-  if (TRI->isVirtualRegister(RegNo)) {
+  if (TargetRegisterInfo::isVirtualRegister(RegNo)) {
     OutStreamer.AddComment(Twine("implicit-def: ") +
----------------
andrew.w.kaylor wrote:
> This addresses warning C4189: local variable is initialized but not referenced.
> 
> Because isVirtualRegister() is a static function, the TRI variable was not actually referenced.
The code change is good, the motivation is questionable - I'd probably fix the code but still disable the warning. A clang-tidy check for calling static functions from a non-static context might be nice to have, but it doesn't really raise to the level of compiler warning.

================
Comment at: tools/llvm-c-test/metadata.c:21
@@ -21,1 +20,3 @@
+  LLVMValueRef values[1];
+  values[0] = LLVMConstInt(LLVMInt32Type(), 0, 0);
 
----------------
andrew.w.kaylor wrote:
> This addresses warning C4204: nonstandard extension used : non-constant aggregate initializer.
Clang has a warning for this (-Wc99-extensions) but we don't enable it, so I'd say it's reasonable to disable MSVC's equivalent for consistency. Unless someone wants to look at turning Clang's version on & seeing what breaks...

http://reviews.llvm.org/D8572

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