Thread (1 message) 1 message, 1 author, 2016-06-15

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

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:58:31

Jeff King [off-list ref] writes:
On Mon, Aug 26, 2013 at 04:59:01PM -0400, Jeff King wrote:
quoted
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.
quoted hunk
-- >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 <redacted>
---
 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);
 			}
 		}
 	}
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help