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

Tue Mar 24 10:42:38 PDT 2015

On Tue, Mar 24, 2015 at 9:59 AM, Aaron Ballman <aaron at aaronballman.com>
wrote:

> On Tue, Mar 24, 2015 at 12:51 PM, David Blaikie <dblaikie at gmail.com>
> wrote:
> > 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.
>
> Yup, I am okay with disabling this one.
>
> >
> > ================
> > 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.
>
> Oye, that's a tough one. I agree with your logic, but at the same
> time, initialized variables that are unused is a bad code smell. In
> this case, the warning found two true positives, for instance.
>

I think it's probably best to catch the two true positives with a different
warning (it's not obvious from the code that they're unused - since they're
used to call (albeit static) member functions). I think it'd be reasonable
to add such a warning to clang, but it doesn't seem like a high priority
(given we only had two instances in all of Clang - well, two instances that
involved otherwise unused variables - but we could catch the rest too). But
it'd probably just be a clang-tidy check, it's not really a bug to use a
static function as a member function call - it's just a bit weird.

>
> >
> > ================
> > 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.
>
> Not everyone runs clang-tidy (unless we have a bot that would catch
> these issues). Also, that diagnostic triggers for other true
> positives, like:
>
> void f() {
>   int i = 12;
>   // Did we forget something here?
> }
>

Clang's -Wunused-variable already catches this, so the codebase isn't going
to end up violating this constraint for very long.

>
> >
> > ================
> > 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...
>
> MSVC's nonstandard extensions tend to be more... interesting... to a
> cross-platform code base. I'd be curious to see what Clang's warning
> catches on our code base and whether we want to consider turning that
> warning on instead of turning MSVC's off. But I don't feel strongly on
> this one since bots are good at catching these problems when they
> really matter.
>

Yeah, I mean if one of these warnings caught cases that would be errors on
another (supported) compiler I'd say we keep it, but otherwise it doesn't
seem too important. Maybe other people have other ideas/like the idea of
being more stringent here (I know compiler-rt compiles with -pedantic, not
sure when/why/how that choice was made).

- David

>
> ~Aaron
>
> >
> > http://reviews.llvm.org/D8572
> >
> > EMAIL PREFERENCES
> >   http://reviews.llvm.org/settings/panel/emailpreferences/
> >
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150324/28bf58bb/attachment.html>