From: Junio C Hamano <gitster@pobox•com>
To: Jeff King <peff@peff•net>
Cc: Lukas Fleischer <git@cryptocrack•de>,
Lance <lancethepants@gmail•com>,
git@vger•kernel.org
Subject: Re: [PATCH] config: do not use C function names as struct members
Date: Mon, 26 Aug 2013 21:38:52 -0700 [thread overview]
Message-ID: <xmqq8uznhhnn.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <20130826215718.GB6219@sigill.intra.peff.net> (Jeff King's message of "Mon, 26 Aug 2013 17:57:18 -0400")
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);
> }
> }
> }
next prev parent reply other threads:[~2013-08-27 4:39 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-08-26 20:10 Issue with compiling git 1.8.4 under uclibc with gcc 4.6.3 Lance
2013-08-26 20:18 ` Lukas Fleischer
2013-08-26 20:29 ` Lance
2013-08-26 20:31 ` Lukas Fleischer
2013-08-26 20:59 ` Jeff King
2013-08-26 21:57 ` [PATCH] config: do not use C function names as struct members Jeff King
2013-08-27 0:56 ` Lukas Fleischer
2013-08-27 4:38 ` Junio C Hamano [this message]
2013-08-26 21:07 ` Issue with compiling git 1.8.4 under uclibc with gcc 4.6.3 Andreas Schwab
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqq8uznhhnn.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=git@cryptocrack$(echo .)de \
--cc=git@vger$(echo .)kernel.org \
--cc=lancethepants@gmail$(echo .)com \
--cc=peff@peff$(echo .)net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox