[LLVMdev] Non-clang warning cleanliness in the LLVM project

[Aaron Ballman]

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