Non-clang warning cleanliness in the LLVM project

Project Infrastructure LLVM Dev List Archives
#1

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
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
#2

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.

#3

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

#4

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.

#5

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