Re: [PATCH 1/2] inline constant return from error() function

[Jeff King <peff@peff.net>]

On Mon, May 12, 2014 at 11:44:26AM -0700, Jonathan Nieder wrote:

> > --- a/git-compat-util.h
> > +++ b/git-compat-util.h
> > @@ -331,7 +331,11 @@ extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)))
> >   * using the function as usual.
> >   */
> >  #if defined(__GNUC__) && ! defined(__clang__)
> > -#define error(...) (error(__VA_ARGS__), -1)
> > +static inline int const_error(void)
> > +{
> > +	return -1;
> > +}
> > +#define error(...) (error(__VA_ARGS__), const_error())
> 
> I wish we could just make error() an inline function that calls some
> print_error() helper, but I don't believe all compilers used to build
> git are smart enough to inline uses of varargs.  So this looks like
> the right thing to do.

Not just "not all compilers", but "not even gcc". If you declare it
always_inline, gcc will even complain "you can't do that, it has
varargs".

For my curiosity, is there any compiler which _does_ inline varargs
calls? Clang would be the obvious guess.

> I kind of wish we weren't in so much of an arms race with static
> analyzers.  Is there some standard way to submit our code as "an idiom
> that should continue not to produce warnings" to be included in a
> testsuite and prevent problems in the future?

I agree the arms race is frustrating, but I think the compiler is being
completely reasonable in this case. It has flagged code where it knows
a variable may be uninitialized depending on the return value from
error(). The only reason I as a human know it's a false positive is that
I know error() will always return -1. So it's not an idiom here, so much
as letting the compiler know everything we know.

It would just be nice if there was an easier way to do it.

I do think giving the compiler that information is nicer than an
attribute saying "trust me, it's initialized". The constant return not
only silences the warnings, but it may actually let the compiler produce
better code (both in these cases, but in the hundreds of other spots
that call error()).

-Peff