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

From: Jeff King <peff@peff.net>
Date: May 12, 2014, 18:56 UTC
Message-ID: <20140512185645.GA32569@sigill.intra.peff.net>
In-Reply-To: <20140512184426.GO9218@google.com>

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

Show 17 quoted lines

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