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

[David Blaikie]

On Tue, Mar 24, 2015 at 1:24 PM, Kaylor, Andrew <andrew.kaylor at intel.com>
wrote:

>  >If your goal is to avoid breaking the Clang -Werror build, I don't
> think there's much you can do other than use a Clang build. But honestly
> enough people don't use it that those of us who do are used to cleaning up
> what gets through, so I wouldn't worry about it too much if I were you.
>
>
>
> My goal is to be able to turn on the “all warnings” option for MSVC
> builds.  I understand that this won’t exactly replicate the Clang warning
> set, but I’m working from the premise that warnings should be fixed because
> it makes the code better.  So even if a /W4 build with MSVC doesn’t report
> everything that Clang would report, I still think it will be helpful.
>
>
>
> I think that as a side effect it has the potential to make breaking the
> Clang -Werror less common, at least for me personally.
>
>
>
> Anyway, if you don’t object, I’ll take out the disputed changes and commit
> the changes we’ve agreed on.  We can see how this evolves from there.
>

Sure, of course.

- Dave


>
>
> -Andy
>
>
>
>
>
> *From:* llvm-commits-bounces at cs.uiuc.edu [mailto:
> llvm-commits-bounces at cs.uiuc.edu] *On Behalf Of *David Blaikie
> *Sent:* Tuesday, March 24, 2015 11:43 AM
> *To:* reviews+D8572+public+921eaa0cb55f1dac at reviews.llvm.org
> *Cc:* llvm-commits at cs.uiuc.edu
> *Subject:* Re: [PATCH] Disable specific MSVC warnings so that W4 warnings
> can be enabled.
>
>
>
>
>
>
>
> On Tue, Mar 24, 2015 at 11:29 AM, Andy Kaylor <andrew.kaylor at intel.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'
> ----------------
> dblaikie wrote:
> > 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.
> These look pretty harmless.  All of the ones I looked at were something
> like these:
>
> ```
> uint8_t(~0U)
> (char)0xFF
> (uint16_t)-1U
>
>
> Yeah, those all look like they do exactly what the user intended... I
> wonder if MSVC expects those to be written some other way? *shrug*
>
>
> ```
>
> ================
> Comment at: lib/Support/APFloat.cpp:1433
> @@ -1432,3 +1432,3 @@
>       an addition or subtraction.  */
> -  subtract ^= sign ^ rhs.sign;
> +  subtract ^= (sign ^ rhs.sign) != 0;
>
> ----------------
> dblaikie wrote:
> > 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.
> OK
>
> ================
> 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 << ", "
> ----------------
> dblaikie wrote:
> > 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.
> If it were just cases like this covered by the warning I would definitely
> agree that it's preferable to disable the warning.  The problem, as Aaron
> Ballman mentioned, is that this same warning reports the basic case.
> ```
> void foo() {
>   int i = 0;
>   // Doesn't use i
> }
> ```
> That's exactly the sort of problem that I want to see when compiling with
> MSVC that motivated me to undertake this change set.
>
>
> If your goal is to avoid breaking the Clang -Werror build, I don't think
> there's much you can do other than use a Clang build. But honestly enough
> people don't use it that those of us who do are used to cleaning up what
> gets through, so I wouldn't worry about it too much if I were you.
>
>
> I hear what you're saying about the change here moving backward, though
> the way the code is written the compiler still has to know that the calls
> above the condition have no side effects to eliminate them.
>
>
> Yep.
>
>
> How about if instead I rewrite the condition like this?
>
> ```
>   if (AlwaysPrintImm0 ||
>       ARM_AM::getAM5Offset(MO2.getImm()) ||
>
>
> Except that ImmOffs is used within the if block too...
>
>
>       ARM_AM::getAM5Op(MO2.getImm()) == ARM_AM::sub) {
> ```
>
> ================
> Comment at: tools/llvm-c-test/metadata.c:21
> @@ -21,1 +20,3 @@
> +  LLVMValueRef values[1];
> +  values[0] = LLVMConstInt(LLVMInt32Type(), 0, 0);
>
> ----------------
> dblaikie wrote:
> > 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...
> I have no objection to disabling this warning.
>
>
> http://reviews.llvm.org/D8572
>
> EMAIL PREFERENCES
>   http://reviews.llvm.org/settings/panel/emailpreferences/
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150324/373edad7/attachment.html>