{"thread":{"id":"43037","subject":"[PATCH 1/2] config.c: avoid duplicated global static variables","startedAt":"2016-08-09T14:16:44Z","lastAt":"2016-08-09T17:17:31Z","messageCount":4,"participants":["Johannes Sixt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"293488","messageId":"57A9E5C1.3030205@kdbg.org","threadId":"43037","inReplyTo":null,"subject":"[PATCH 1/2] config.c: avoid duplicated global static variables","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-08-09T14:16:33Z","receivedAt":"2016-08-09T14:16:44Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Repeating the definition of a static variable seems to be valid in C.\nNevertheless, it is bad style because it can cause confusion, definitely\nwhen it becomes necessary to change the type.\n\nd64ec16 (git config: reorganize to use parseopt, 2009-02-21) added two\nstatic variables near the top of the file config.c without removing the\ndefinitions of the two variables that occurs later in the file.\n\nThe two variables were needed earlier in the file in the newly\nintroduced parseopt structure. These references were removed later in\nd0e08d6 (config: fix parsing of \"git config --get-color some.key -1\",\n2014-11-20).\n\nRemove the redundant, younger, definitions near the top of the file and\nkeep the original definitions that occur later.\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n Why the heck are duplicated static variables not an error in C?\n Since they aren't (as it seems), this and the next patch are just\n a matter of taste.\n\n builtin/config.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex a991a53..6cbf733 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -25,7 +25,6 @@ static char term = '\\n';\n static int use_global_config, use_system_config, use_local_config;\n static struct git_config_source given_config_source;\n static int actions, types;\n-static const char *get_color_slot, *get_colorbool_slot;\n static int end_null;\n static int respect_includes = -1;\n static int show_origin;\n-- \n2.9.2.935.gccae72a.dirty\n\n"},{"id":"293489","messageId":"57A9E614.6050601@kdbg.org","threadId":"43037","inReplyTo":"57A9E5C1.3030205@kdbg.org","subject":"[PATCH 2/2] commit-slab.h: avoid duplicated global static variables","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-08-09T14:17:56Z","receivedAt":"2016-08-09T14:18:05Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"The gigantic define_commit_slab() macro repeats the definition of a\nstatic variable that occurs earlier in the macro text. The purpose of\nthe repeated definition at the end of the macro is that it takes the\nsemicolon that occurs where the macro is used.\n\nWe cannot just remove the first definition of the variable because it\nis referenced elsewhere in the macro text, and defining the macro later\nwould produce undefined identifier errors. We cannot have a \"forward\"\ndeclaration, either. (This works only with \"extern\" global variables.)\n\nThe solution is to use a declaration of a struct that is already defined\nearlier. This language construct can serve the same purpose as the\nduplicated static variable definition, but without the confusion.\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n commit-slab.h | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/commit-slab.h b/commit-slab.h\nindex f84b449..006a50b 100644\n--- a/commit-slab.h\n+++ b/commit-slab.h\n@@ -126,16 +126,16 @@ static MAYBE_UNUSED elemtype *slabname## _peek(struct slabname *s,\t\\\n \treturn slabname##_at_peek(s, c, 0);\t\t\t\t\\\n }\t\t\t\t\t\t\t\t\t\\\n \t\t\t\t\t\t\t\t\t\\\n-static int stat_ ##slabname## realloc\n+struct slabname\n \n /*\n- * Note that this seemingly redundant second declaration is required\n+ * Note that this redundant forward declaration is required\n  * to allow a terminating semicolon, which makes instantiations look\n  * like function declarations.  I.e., the expansion of\n  *\n  *    define_commit_slab(indegree, int);\n  *\n- * ends in 'static int stat_indegreerealloc;'.  This would otherwise\n+ * ends in 'struct indegree;'.  This would otherwise\n  * be a syntax error according (at least) to ISO C.  It's hard to\n  * catch because GCC silently parses it by default.\n  */\n-- \n2.9.2.935.gccae72a.dirty\n\n"},{"id":"293490","messageId":"57A9E7D7.8030704@kdbg.org","threadId":"43037","inReplyTo":"57A9E614.6050601@kdbg.org","subject":"Re: [PATCH 2/2] commit-slab.h: avoid duplicated global static variables","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-08-09T14:25:27Z","receivedAt":"2016-08-09T14:25:34Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"BTW, these are all instances of duplicated global static variables that \ncan be found in a standard Linux build.\n\nHow I found them? I waded through the error messages produced by compiling \nthe code base as C++ code for the fun of it (basically CFLAGS='-x c++ \n-fpermissive').\n\n-- Hannes\n\n"},{"id":"293501","messageId":"xmqqpophc15o.fsf@gitster.mtv.corp.google.com","threadId":"43037","inReplyTo":"57A9E614.6050601@kdbg.org","subject":"Re: [PATCH 2/2] commit-slab.h: avoid duplicated global static variables","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-09T17:17:23Z","receivedAt":"2016-08-09T17:17:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n>   * like function declarations.  I.e., the expansion of\n>   *\n>   *    define_commit_slab(indegree, int);\n>   *\n> - * ends in 'static int stat_indegreerealloc;'.  This would otherwise\n> + * ends in 'struct indegree;'.  This would otherwise\n>   * be a syntax error according (at least) to ISO C.  It's hard to\n>   * catch because GCC silently parses it by default.\n>   */\n\nI briefly wondered what \"otherwise\" and \"it\" in \"silently parses it\"\nrefer to; it is having a hanging semicolon after the definition of\nthe *_peek() function, i.e.\n\n\t...\n        static int *indegree_peek(...)\n        {\n\t\treturn indegree_at_peek(s, c, 0);\n\t}\n\t;\n\nso all is fine.\n\nNeedless to say, using \"struct slabname;\" is indeed a much nicer\nsolution you found to the original issue.  Thanks.\n"}]}