Re: [PATCH 1/5] notes-utils: handle boolean notes.rewritemode correctly

From: Jeff King <peff@peff.net>
Date: Feb 18, 2014, 07:46 UTC
Message-ID: <20140218074632.GA29804@sigill.intra.peff.net>
In-Reply-To: <87txbzvxgq.fsf@fencepost.gnu.org>

On Sun, Feb 16, 2014 at 05:22:45PM +0100, David Kastrup wrote:

Show 14 quoted lines

> Not really relevant to this patch, but looking at the output of
> 
> git grep config_error_nonbool
> 
> seems like a serious amount of ridiculousness going on.  The header
> shows
> 
> cache.h:extern int config_error_nonbool(const char *);
> cache.h:#define config_error_nonbool(s) (config_error_nonbool(s), -1)
> 
> and the implementation
> 
> config.c:#undef config_error_nonbool
> config.c:int config_error_nonbool(const char *var)

You could always look in the commit history:

  $ git log -S'#define config_error_nonbool' cache.h

or search the mailing list:

  http://thread.gmane.org/gmane.comp.version-control.git/211505

> Presumably this was done so that the uses of config_error_nonbool can be
> recognized as returning -1 unconditionally.

Yes, it helps prevent false positives in gcc's flow analysis (specifically, -Wuninitialized warnings).

> But is that worth the obfuscation?

Yes?

> Why not let config_error_nonbool return -1 in the first place?

It originally did, but callers do not get to see the static return, so they miss some analysis opportunities.

> It does not appear like any caller would call the function rather than
> the macro, so why declare the function as returning an int at all?

Because we don't use these macros on non-gnu compilers; they actually see the real function return.

> And why give it the same name as the macro (risking human/computer
> confusion and requiring an explicit #undef for the definition or was
> that declaration?) instead of config_error_nonbool_internal or
> whatever else?

This particular case is simple, but see error() for another use of the same technique which requires variadic macros, which are not available everywhere. Callers want to say just "error(...)", so we have to name the macro that. If we call the matching function "error_internal", then non-gnu compilers would need a macro to convert "error(...)" to "error_internal(...)". But they can't do so, because it would be a variadic macro.

So you could adjust config_error_nonbool(), but not error(). But that introduces its own confusion, because the technique is not applied consistently.

I agree the #define is ugly. But it's confined to a small area, and it does solve an actual problem.

-Peff