This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Add macros for diagnostic control, use them in locale/weightwc.h


When I said I wanted a macro, what I had in mind was a single macro
that would wrap the affected code, e.g.

	SUPPRESS_WARNING ("maybe-uninitialized",
			  ... code inside here ...
			 )

My rationale was to structure the source code such that it would be
difficult to accidentally spread the suppression zone across more code
than really needed it (or at least be easy to be lazy and still not do
that).

Paul took my strawman "tightest possible scope" to its absurd extreme.
That maybe have been a bit of baiting on my part.  I always expected
we would find a balance between the literal tighest possible scope and
a scope that is tight enough to minimize the risk of masking warnings
we actually want to see (especially as the code is changed later)
while not making the code so contorted that it really hampers
maintainability in another way.

If GCC's limitations make it infeasible to narrow the suppression
scope very much, and/or make it impossible to use a surrounding macro
to do so like my idea, then those ideas are pretty much out the
window.  The pragma syntax is a bit verbose and clunky, so some macros
to make things more concise are still probably worthwhile.

The bottom line is that I want it to be tractable and not gratuitously
cumbersome to suppress particular warnings, but I want it to be
difficult to do so indiscriminately or accidentally and easiest to
maintain and change the code such that we won't inadvertently have
warnings suppressed for new code.  I'm far less concerned about
warnings in test code.  So, if outside of test code we in fact have a
very small number of warnings in need of suppression, perhaps I've
made a mountain of a molehill.

Regardless of the mechanics we settle on, I want to say some things
about review standards for new warning suppressions.  The bar to
adding a warning suppression should be quite high.  It should be
especially high for the "find a bug via control-flow analysis" class
of warnings such as -Wmaybe-uninitialized.  The weightwc.h example in
Joseph's patch has a little of the important information, but IMHO
it's almost a perfect example of what not to do.  It says under what
conditions the warning was observed (machine, compiler version), which
is important information.  

What's at least as important is the rationale by which we're confident
that the compiler is wrong and the code is right.  Just because a
warning has been there for a long time, and we've let it pass, and we
aren't aware of a bug in the code, does not mean that there isn't
actually a bug in the code.  I looked at the fnmatch/weightwc case a
while ago and after a few minutes of looking at the code (with which I
was not especially familiar) could not easily convince myself that I
knew for sure that use of an uninitialized value was impossible.
Joseph has since described his own analysis of the code and made the
case that it is in fact safe.  An explanation of that analysis and the
"proof" (informally speaking) of why the warning is wrong is what
should be required in comments at the site of a warning suppression.

When someone takes the time to do such analysis, he will then be close
enough to the code that he might see ways to change the code so that
the warning goes away without a suppression, as Paul did for this
case.  That is always the best possible result, and what we should be
striving for.  Accepting that a warning suppression is correct should
be the penultimate resort, and never done lightly (the last one, that
we never get to, being pessimizing the code with dead stores and the
like).


Thanks,
Roland


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]