Re: [PATCH] config: do not use C function names as struct members

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> On Mon, Aug 26, 2013 at 04:59:01PM -0400, Jeff King wrote:
>
>> Hmm. I wonder if fgetc is a macro in uclibc? Just grepping their
>> stdio.h, it looks like that is a possibility.
>> 
>> I think they are probably wrong to do so (but I'd have to check the
>> standard). However, the cleaner workaround would probably be to call the
>> fgetc struct member something else.
>
> Nope, it's allowed. I think we should do the patch below:

Thanks.

> -- >8 --
> Subject: config: do not use C function names as struct members
>
> According to C99, section 7.1.4:
>
>   Any function declared in a header may be additionally
>   implemented as a function-like macro defined in the
>   header.
>
> Therefore calling our struct member function pointer "fgetc"
> may run afoul of unwanted macro expansion when we call:
>
>   char c = cf->fgetc(cf);
>
> This turned out to be a problem on uclibc, which defines
> fgetc as a macro and causes compilation failure.
>
> The standard suggests fixing this in a few ways:
>
>   1. Using extra parentheses to inhibit the function-like
>      macro expansion. E.g., "(cf->fgetc)(cf)". This is
>      undesirable as it's ugly, and each call site needs to
>      remember to use it (and on systems without the macro,
>      forgetting will compile just fine).
>
>   2. Using #undef (because a conforming implementation must
>      also be providing fgetc as a function). This is
>      undesirable because presumably the implementation was
>      using the macro for a performance benefit, and we are
>      dropping that optimization.
>
> Instead, we can simply use non-colliding names.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  config.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/config.c b/config.c
> index e13a7b6..9f9bf0c 100644
> --- a/config.c
> +++ b/config.c
> @@ -27,9 +27,9 @@ struct config_source {
>  	struct strbuf value;
>  	struct strbuf var;
>  
> -	int (*fgetc)(struct config_source *c);
> -	int (*ungetc)(int c, struct config_source *conf);
> -	long (*ftell)(struct config_source *c);
> +	int (*do_fgetc)(struct config_source *c);
> +	int (*do_ungetc)(int c, struct config_source *conf);
> +	long (*do_ftell)(struct config_source *c);
>  };
>  
>  static struct config_source *cf;
> @@ -217,13 +217,13 @@ static int get_next_char(void)
>  
>  static int get_next_char(void)
>  {
> -	int c = cf->fgetc(cf);
> +	int c = cf->do_fgetc(cf);
>  
>  	if (c == '\r') {
>  		/* DOS like systems */
> -		c = cf->fgetc(cf);
> +		c = cf->do_fgetc(cf);
>  		if (c != '\n') {
> -			cf->ungetc(c, cf);
> +			cf->do_ungetc(c, cf);
>  			c = '\r';
>  		}
>  	}
> @@ -992,9 +992,9 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)
>  		top.u.file = f;
>  		top.name = filename;
>  		top.die_on_error = 1;
> -		top.fgetc = config_file_fgetc;
> -		top.ungetc = config_file_ungetc;
> -		top.ftell = config_file_ftell;
> +		top.do_fgetc = config_file_fgetc;
> +		top.do_ungetc = config_file_ungetc;
> +		top.do_ftell = config_file_ftell;
>  
>  		ret = do_config_from(&top, fn, data);
>  
> @@ -1013,9 +1013,9 @@ int git_config_from_buf(config_fn_t fn, const char *name, const char *buf,
>  	top.u.buf.pos = 0;
>  	top.name = name;
>  	top.die_on_error = 0;
> -	top.fgetc = config_buf_fgetc;
> -	top.ungetc = config_buf_ungetc;
> -	top.ftell = config_buf_ftell;
> +	top.do_fgetc = config_buf_fgetc;
> +	top.do_ungetc = config_buf_ungetc;
> +	top.do_ftell = config_buf_ftell;
>  
>  	return do_config_from(&top, fn, data);
>  }
> @@ -1196,7 +1196,7 @@ static int store_aux(const char *key, const char *value, void *cb)
>  				return 1;
>  			}
>  
> -			store.offset[store.seen] = cf->ftell(cf);
> +			store.offset[store.seen] = cf->do_ftell(cf);
>  			store.seen++;
>  		}
>  		break;
> @@ -1223,19 +1223,19 @@ static int store_aux(const char *key, const char *value, void *cb)
>  		 * Do not increment matches: this is no match, but we
>  		 * just made sure we are in the desired section.
>  		 */
> -		store.offset[store.seen] = cf->ftell(cf);
> +		store.offset[store.seen] = cf->do_ftell(cf);
>  		/* fallthru */
>  	case SECTION_END_SEEN:
>  	case START:
>  		if (matches(key, value)) {
> -			store.offset[store.seen] = cf->ftell(cf);
> +			store.offset[store.seen] = cf->do_ftell(cf);
>  			store.state = KEY_SEEN;
>  			store.seen++;
>  		} else {
>  			if (strrchr(key, '.') - key == store.baselen &&
>  			      !strncmp(key, store.key, store.baselen)) {
>  					store.state = SECTION_SEEN;
> -					store.offset[store.seen] = cf->ftell(cf);
> +					store.offset[store.seen] = cf->do_ftell(cf);
>  			}
>  		}
>  	}