llvm dev - Mar 2015 - [LLVMdev] Non-clang warning cleanliness in the LLVM project

So a while back we took a moderately aggressive stance on disabling GCC
warnings with false positives, specifically those related to uninitialized
variables. In cases where GCC suggested initializing a variable yet the
algorithm was safely initializing the variable, adding the GCC-suggested
initialization could thwart tools like MSan/ASan. So it was suggested that
we should not abide by GCC's warning and rely on Clang's more carefully
limited warning without the problematic false positives.

Recently Andy Kaylor's been working on getting the MSVC build warning clean
by a combination of disabling warnings and fixing a few cases of relatively
low frequency.

I've generally been encouraging people to aggressively disable non-clang
warnings whenever there's a false positive (anything other than a bug, or
at least anything that doesn't strictly improve readability) and one such
instance of this is in the review for r233088 which amounts to something
like:

template<typename T>
int func(T t) {
  return t.func() >> 8;
}

(it's more complicated than that, but this is the crux of it) - for some
instantiations of this template, T::func returns an 8-bit type, and MSVC
warns that the shift will produce a zero value. The code is correct (the
author intended this behavior, because some instantiations produce a
wider-than-8-bit- type and the higher bits are desired, if present).

I suggested disabling this warning, but that would mean we miss out on the
true positives as well (Clang doesn't seem to have any warning like this),
though we haven't seen these very often.

How do people feel about working around this warning so we can keep it
enabled?
How do people feel about disabling this warning?
How do people feel about disabling other non-Clang warnings which have
false positives?

(in the case of Clang warnings with false positives we should take the same
approach, except we also have the option to improve the quality of the
warning by fixing Clang, which is what we generally try to do)

- David
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150327/577c4c7f/attachment.html>

It's fairly quick to workaround a warning compared with much more time
identifying and debugging a real bug, so in total it is more efficient to
keep even high false positive rate warnings enabled.


2015-03-27 20:31 GMT+03:00 David Blaikie <dblaikie at gmail.com>:
> So a while back we took a moderately aggressive stance on disabling GCC
> warnings with false positives, specifically those related to uninitialized
> variables. In cases where GCC suggested initializing a variable yet the
> algorithm was safely initializing the variable, adding the GCC-suggested
> initialization could thwart tools like MSan/ASan. So it was suggested that
> we should not abide by GCC's warning and rely on Clang's more
carefully
> limited warning without the problematic false positives.
>
> Recently Andy Kaylor's been working on getting the MSVC build warning
> clean by a combination of disabling warnings and fixing a few cases of
> relatively low frequency.
>
> I've generally been encouraging people to aggressively disable
non-clang
> warnings whenever there's a false positive (anything other than a bug,
or
> at least anything that doesn't strictly improve readability) and one
such
> instance of this is in the review for r233088 which amounts to something
> like:
>
> template<typename T>
> int func(T t) {
>   return t.func() >> 8;
> }
>
> (it's more complicated than that, but this is the crux of it) - for
some
> instantiations of this template, T::func returns an 8-bit type, and MSVC
> warns that the shift will produce a zero value. The code is correct (the
> author intended this behavior, because some instantiations produce a
> wider-than-8-bit- type and the higher bits are desired, if present).
>
> I suggested disabling this warning, but that would mean we miss out on the
> true positives as well (Clang doesn't seem to have any warning like
this),
> though we haven't seen these very often.
>
> How do people feel about working around this warning so we can keep it
> enabled?
> How do people feel about disabling this warning?
> How do people feel about disabling other non-Clang warnings which have
> false positives?
>
> (in the case of Clang warnings with false positives we should take the
> same approach, except we also have the option to improve the quality of the
> warning by fixing Clang, which is what we generally try to do)
>
> - David
>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150327/2e75348e/attachment.html>

Hi David,

I think your guidance about disabling warnings for scenarios that Clang does not
care to warn about is generally good.

The gray area, to my way of thinking, is warnings that in the general case Clang
does report but which have special cases that are reported by other compilers
and not by Clang.  We saw this with the warning about local variables that were
initialized but never referenced.  I wouldn’t want to disable that warning
across the board for MSVC because it is a useful warning.  In that particular
case, I think it would be useful for Clang to also report the case where an
object variable is only ever used to call static functions in its class.  The
case where specific template instantiations could cause the warning is more
problematic, and sounds like the shift warning you describe below.

My idealistic preference would be to leave these gray-area warnings enabled.  In
the case of the initialized-but-not used warnings I just mentioned, I feel like
all of the changes I made to avoid the warnings resulted in objectively better
code (though that wasn’t the case with my initial attempt to fix one of them). 
On the other hand, I think it’s likely that cases will arise from time-to-time
where a non-clang warning can’t be corrected with a change that we would want to
make.  My personal preference would be to insert a compiler-specific pragma to
suppress these warnings on a case-by-case basis.  I don’t think it will be
common.

Of course, this won’t stick if we don’t have buildbots that are looking for
clean compiles with non-clang compilers.  I don’t know what the state of GCC
buildbots is in this respect.  Obviously what few MSVC builders we have aren’t
doing it yet, but I’d like to at least enable the “all warnings” option by
default for Windows very soon.

-Andy


From: David Blaikie [mailto:dblaikie at gmail.com]
Sent: Friday, March 27, 2015 10:32 AM
To: LLVM Developers Mailing List
Cc: Aaron Ballman; Chandler Carruth; Simon Atanasyan; Kaylor, Andrew
Subject: Non-clang warning cleanliness in the LLVM project

So a while back we took a moderately aggressive stance on disabling GCC warnings
with false positives, specifically those related to uninitialized variables. In
cases where GCC suggested initializing a variable yet the algorithm was safely
initializing the variable, adding the GCC-suggested initialization could thwart
tools like MSan/ASan. So it was suggested that we should not abide by GCC's
warning and rely on Clang's more carefully limited warning without the
problematic false positives.

Recently Andy Kaylor's been working on getting the MSVC build warning clean
by a combination of disabling warnings and fixing a few cases of relatively low
frequency.

I've generally been encouraging people to aggressively disable non-clang
warnings whenever there's a false positive (anything other than a bug, or at
least anything that doesn't strictly improve readability) and one such
instance of this is in the review for r233088 which amounts to something like:

template<typename T>
int func(T t) {
  return t.func() >> 8;
}

(it's more complicated than that, but this is the crux of it) - for some
instantiations of this template, T::func returns an 8-bit type, and MSVC warns
that the shift will produce a zero value. The code is correct (the author
intended this behavior, because some instantiations produce a wider-than-8-bit-
type and the higher bits are desired, if present).

I suggested disabling this warning, but that would mean we miss out on the true
positives as well (Clang doesn't seem to have any warning like this), though
we haven't seen these very often.

How do people feel about working around this warning so we can keep it enabled?
How do people feel about disabling this warning?
How do people feel about disabling other non-Clang warnings which have false
positives?

(in the case of Clang warnings with false positives we should take the same
approach, except we also have the option to improve the quality of the warning
by fixing Clang, which is what we generally try to do)

- David
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150327/aeaa261c/attachment.html>

On Fri, Mar 27, 2015 at 10:31 AM, David Blaikie <dblaikie at gmail.com>
wrote:
> How do people feel about working around this warning so we can keep it
> enabled?
>
Have we found any bugs with it? It doesn't seem *that* useful.

If we have, I'd rather implement a version in Clang that was crafty and
tried to not fire on template instantiations where it would be noisy.

> How do people feel about disabling this warning?
>
Unless its finding bugs, sure.

> How do people feel about disabling other non-Clang warnings which have
> false positives?
>
I'm one of those espousing the policy of disabling warnings that aren't
finding bugs. That's because I never want people to avoid or delay fixing
warnings under the idea that it isn't *really* a problem.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150327/901e22e7/attachment.html>

On Fri, Mar 27, 2015 at 1:31 PM, David Blaikie <dblaikie at gmail.com>
wrote:
> So a while back we took a moderately aggressive stance on disabling GCC
> warnings with false positives, specifically those related to uninitialized
> variables. In cases where GCC suggested initializing a variable yet the
> algorithm was safely initializing the variable, adding the GCC-suggested
> initialization could thwart tools like MSan/ASan. So it was suggested that
> we should not abide by GCC's warning and rely on Clang's more
carefully
> limited warning without the problematic false positives.
MSVC has some warnings that are of incredibly dubious value (C4800
'type' : forcing value to bool 'true' or 'false'
(performance warning)
being a classic example) that I am happy to disable globally. So I'm
not opposed to blanket disabling of overly chatty warnings in general.

As someone who has personally spent time fixing MSVC warnings that
crop up, it's almost invariably trivial to fix false positives, and
can sometimes be enlightening even when there's a false positive
because it may be pointing out a code smell that requires better
comments, or can be improved in some other way. I don't see
considerable benefit to being aggressive with disabling warnings
because of a few false positives. I do see detriment to disabling
warnings with a shotgun approach. My biggest concerns are:

* Missing out on true-positives that may not be covered by Clang
warnings (even if they're rare)
* Parts of the project which are reasonably used out-of-tree, such as
ADT, that stop compiling cleanly for default-enabled warnings in MSVC

To me, the watermark that makes more sense is when a warning's value
is outweighed by the effort required to fix false-positives for it.
For some warnings, that may be the first instance of seeing it (C4800
comes to mind). For other warnings, they may find a true positive now
and again, but if new code adds a dozen false positives every week
that you have to wade through, I don't think that's time well spent
(C4456 'declaration of 'var' hides local variable' comes to
mind).
>
> Recently Andy Kaylor's been working on getting the MSVC build warning
clean
> by a combination of disabling warnings and fixing a few cases of relatively
> low frequency.
>
> I've generally been encouraging people to aggressively disable
non-clang
> warnings whenever there's a false positive (anything other than a bug,
or at
> least anything that doesn't strictly improve readability) and one such
> instance of this is in the review for r233088 which amounts to something
> like:
>
> template<typename T>
> int func(T t) {
>   return t.func() >> 8;
> }
>
> (it's more complicated than that, but this is the crux of it) - for
some
> instantiations of this template, T::func returns an 8-bit type, and MSVC
> warns that the shift will produce a zero value. The code is correct (the
> author intended this behavior, because some instantiations produce a
> wider-than-8-bit- type and the higher bits are desired, if present).
>
> I suggested disabling this warning, but that would mean we miss out on the
> true positives as well (Clang doesn't seem to have any warning like
this),
> though we haven't seen these very often.
>
> How do people feel about working around this warning so we can keep it
> enabled?
I think that the warning provides value, but not in that particular
case identified. Since there are several ways we can silence that
warning (with various tradeoffs to them), I don't see a need to
disable the warning.
> How do people feel about disabling this warning?
I'm opposed to it because it can catch real bugs, and we do a fair
amount of bit twiddling in our code (including in ADT).
> How do people feel about disabling other non-Clang warnings which have
false
> positives?
I'm opposed to it because they can catch real bugs, with the caveat
that any warning which is *mostly* false positives that are better
covered by Clang warnings (with lower false positives), I am happy to
globally disable. Not everyone is able to bootstrap with Clang, and
from what I've seen from trying to keep bots warning-free, most people
ignore new warnings being added to the bots, too (IIRC, there's
resistance to the idea of -Werror bots?)

~Aaron