In practice, false positives (and negatives) of both kinds, whether
they fit the formal definition or the informal one, are the nature
of virtually all non-trivial static diagnostics, certainly all those
that depend on control or data flow analysis. Some are due to bugs
or limitations in the implementation of the warning. Others are
inherent in the technology.
Yes, and I argue that these warnings belong in a different "level" of
warnings than the trivial warnings.
Introducing more levels sounds fine to me. I wouldn't want to see
a more permissive default; my preference would be the opposite,
leaving it to projects to adjust.
Lastly, in the case of uninitialized variables, the usual solution
of initializing them is trivial and always safe (some coding styles
even require it).
Here it shows that we don't work with the same type of code at all. If I
am using a boost::optional, i.e. a class with a buffer and a boolean
that says if the buffer is initialized, how do I initialize the
(private) buffer? Or should boost itself zero out the buffer whenever
the boolean is set to false? The variables can easily be hidden behind
dozens of levels of abstraction that make them hard to initialize, and
there may be nothing meaningful to initialize them with. Uninitialized
also includes clobbered (out-of-scope for instance) in gcc, where it
isn't clear what you are supposed to initialize to quiet the warning.
You're right that this is hard to imagine without first hand experience
with the problem. If this is a common pattern with the warning in C++
class templates in general, a representative test case would help get
a better appreciation of the problem and might also give us an idea
of a better solution. (If there is one in Bugzilla please point me
at it.)
This message concentrates on the negatives, but that doesn't mean I
consider -Wmaybe-uninitialized as useless. It can find true
uninitialized uses. And even some false positives can point at places
where we can help the compiler generate better code (say with a
default __builtin_unreachable case in a switch). I did in the past
contribute patches to make it warn more often, and I might do so
again in the future.
This paragraph makes me think you equate false positives from this
warning with those from -Wuninitialized.
Uh? No, I don't know what gave you this impression.
It's common to view as a false positive (or "bad" as you write below)
every instance of a warning designed to detect or prevent bugs that
doesn't indicate one (as opposed to warnings designed to help write
better/clearer code like -Wparentheses).
"May be unitialized" doesn't preclude the possibility that the
variable is not used uninitialized.
But if the variable can never be used uninitialized and gcc is simply
unable to prove it, the warning is bad. It may not be a "false positive"
depending on your definition, but it is undesirable.
Ideally each instance of every warning designed to find bugs would
point out one. But 100% accuracy or a zero rate of the undesirable
instances is not attainable in general, and reducing their rate at
the expense of the helpful ones also isn't the best tradeoff either.
The challenge is striking the right balance between their ratios.
(Only if that can't done then it might be time to consider
disabling/demoting the warning. I don't have the impression
we are at that point with -Wmaybe-uninitialized but I haven't
done any research.)
My opinion is that -Wmaybe-uninitialized would serve its purpose
better as part of -Wextra. People tend to use -Wall with -Werror
(either explicitly or implicitly by modifying the code until all
warnings are gone). What I see currently in projects where I
participate is that -Wmaybe-uninitialized is making things worse.
People don't investigate deeply the cause of the warning, they just
go for whatever "quick-fix" makes the compiler shut up. Quite often,
this involves extra code that is less readable and performs worse,
while it didn't even "fix" what caused the warning, it just randomly
ended up with code where the compiler doesn't warn (possibly because
the function got bigger, which changed inlining decisions...).
I agree that we need to be mindful of warnings leading to unnecessary
and perhaps inappropriate code changes (as the author of a few warnings
with this potential it has been on mind a lot).
At the same time, not taking the time to properly analyze diagnostics
and come up with the appropriate solutions seems like a problem that's
orthogonal to the warnings themselves. Disabling warnings in hopes of
solving what's really a problem with the development process or
a culture of a subset of projects doesn't seem like an optimal solution.
I agree with that. But if I complain about the mindless quickfixes, the
reply is that customers ask for a library that does not warn, and the
developers do not have infinite time, so they do the minimum to avoid
warnings. This is strongly linked to the warning levels.
One could recommend that customers include the library with -isystem to
disable the warnings inside the library, bug that's fragile (though it
has gotten quite good), and most importantly it also disables some
warnings that are relevant to the user.
Right, that wouldn't be much better than disabling the warning on
the command line (or us removing it from -Wall and perhaps also
from -Wextra).
So since mindless quick fixes aren't the way to go and assuming we
agree that warnings have value even with some noise, is demoting
them to lower levels because they're not always used properly
the best solution? No predetermined system of warning levels is
going to make everyone happy. Different projects have different
constraints and tolerances for noise, or even resources to fix
real bugs -- GCC with over 400 wrong-code bugs in Open status
being a case in point, so a level that works for one, like
a library, may be overly pedantic for an application.
More warning levels might help. Warning profiles (say one for
system libraries or freestanding code like the kernel, another
for C++ class libraries, yet another for applications) might
be another approach. Others might be worth brainstorming about.
If the warning is not enabled by default so much but only when people
are ready to investigate any warning thoroughly, the quickfix
mentality is less likely to be present. People using
-Wmaybe-uninitialized need to be willing to ignore false positives,
or disable them with pragmas.
Note that similar arguments may apply to some other warnings that
somehow made their way into -Wall when they shouldn't have, but for
now I am only proposing to move -Wmaybe-uninitialized. Some people
tend to consider that if a warning is not part of -Wall, it might as
well not exist. Obviously I disagree with that.
The description of -Wall is vague enough that arguments about which
warnings should or shouldn't be enabled by it are unavoidably subject
to individual biases and unlikely to lead to a consensus, either among
ourselves or, even less likely, among users.
True. To me, one of the key documented properties of -Wall is being
"easy to avoid". -Wall -Werror should be a usable combination. Anything
not easy to avoid belongs in a different class, say -Wextra. We can
rename that to W1/W2/... if Wall is too charged with historical meaning.
I would have expected dealing with -Wmaybe-uninitialized to always
be easy. Your comment about std::optional suggests there are cases
when it isn't. Can we look at some of those cases and see if there
is something we can do to make it easier?
A shared definition of a false positive should be one of the very
first steps to coming closer to a consensus. Real world (as opposed
to anecdotal) data on the rates of actual rates of false positives and
negatives vs true positives would be also most helpful, as would some
consensus of the severity of the bugs the true positives expose, as
well as some objective measure of the ease of suppression. There
probably are others but these would be a start.
This data is going to be super hard to get. Most projects have been
compiling for years and tweaking their code to avoid some warnings. We
do not get to see the code that people originally write, we can only see
what they commit.
It's not perfect but we should be able to get a rough idea based
on bug reports in Bugzilla. Many distros rebuild the world before
a GCC release and new instances of build-breaking warnings tend to
get filed there. Meta-bugs can be helpful as the first step.
A finer-grained classification by search terms (e.g., "bogus"
"spurious", etc.) helps narrow it down even more.
Martin
