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

[David Blaikie]

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/21cf3b43/attachment.html>