{"thread":{"id":"48831","subject":"[PATCH] builtin/config: work around an unsized array forward declaration","startedAt":"2018-07-05T18:44:15Z","lastAt":"2018-07-07T23:58:29Z","messageCount":7,"participants":["Beat Bolli","Taylor Blau","Jeff King","Junio C Hamano","Kim Gybels"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"351710","messageId":"20180705183445.30901-1-dev+git@drbeat.li","threadId":"48831","inReplyTo":null,"subject":"[PATCH] builtin/config: work around an unsized array forward declaration","fromName":"Beat Bolli","fromEmail":"dev+git@drbeat.li","sentAt":"2018-07-05T18:34:45Z","receivedAt":"2018-07-05T18:44:15Z","isPatch":true,"sender":{"key":"dev+git@drbeat.li","avatar":"https://avatars.githubusercontent.com/u/21444?v=4"},"body":"As reported here[0], Microsoft Visual Studio 2017.2 and \"gcc -pedantic\"\ndon't understand the forward declaration of an unsized static array.\nThey insist on an array size:\n\n    d:\\git\\src\\builtin\\config.c(70,46): error C2133: 'builtin_config_options': unknown size\n\nThe thread [1] explains that this is due to the single-pass nature of\nold compilers.\n\nTo work around this error, introduce the forward-declared function\nusage_builtin_config() instead that uses the array\nbuiltin_config_options only after it has been defined.\n\nAlso use this function in all other places where usage_with_options() is\ncalled with the same arguments.\n\n[0]: https://github.com/git-for-windows/git/issues/1735\n[1]: https://groups.google.com/forum/#!topic/comp.lang.c.moderated/bmiF2xMz51U\n\nFixes https://github.com/git-for-windows/git/issues/1735\n\nReported-By: Karen Huang (via GitHub)\nSigned-off-by: Beat Bolli <dev+git@drbeat.li>\n---\n builtin/config.c | 27 +++++++++++++++------------\n 1 file changed, 15 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex b29d26dede..2c93a289a7 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -67,7 +67,7 @@ static int show_origin;\n \t{ OPTION_CALLBACK, (s), (l), (v), NULL, (h), PARSE_OPT_NOARG | \\\n \tPARSE_OPT_NONEG, option_parse_type, (i) }\n \n-static struct option builtin_config_options[];\n+static NORETURN void usage_builtin_config(void);\n \n static int option_parse_type(const struct option *opt, const char *arg,\n \t\t\t     int unset)\n@@ -111,8 +111,7 @@ static int option_parse_type(const struct option *opt, const char *arg,\n \t\t * --type=int'.\n \t\t */\n \t\terror(\"only one type at a time.\");\n-\t\tusage_with_options(builtin_config_usage,\n-\t\t\tbuiltin_config_options);\n+\t\tusage_builtin_config();\n \t}\n \t*to_type = new_type;\n \n@@ -157,11 +156,16 @@ static struct option builtin_config_options[] = {\n \tOPT_END(),\n };\n \n+static NORETURN void usage_builtin_config(void)\n+{\n+\tusage_with_options(builtin_config_usage, builtin_config_options);\n+}\n+\n static void check_argc(int argc, int min, int max) {\n \tif (argc >= min && argc <= max)\n \t\treturn;\n \terror(\"wrong number of arguments\");\n-\tusage_with_options(builtin_config_usage, builtin_config_options);\n+\tusage_builtin_config();\n }\n \n static void show_config_origin(struct strbuf *buf)\n@@ -596,7 +600,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \tif (use_global_config + use_system_config + use_local_config +\n \t    !!given_config_source.file + !!given_config_source.blob > 1) {\n \t\terror(\"only one config file at a time.\");\n-\t\tusage_with_options(builtin_config_usage, builtin_config_options);\n+\t\tusage_builtin_config();\n \t}\n \n \tif (use_local_config && nongit)\n@@ -660,12 +664,12 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \n \tif ((actions & (ACTION_GET_COLOR|ACTION_GET_COLORBOOL)) && type) {\n \t\terror(\"--get-color and variable type are incoherent\");\n-\t\tusage_with_options(builtin_config_usage, builtin_config_options);\n+\t\tusage_builtin_config();\n \t}\n \n \tif (HAS_MULTI_BITS(actions)) {\n \t\terror(\"only one action at a time.\");\n-\t\tusage_with_options(builtin_config_usage, builtin_config_options);\n+\t\tusage_builtin_config();\n \t}\n \tif (actions == 0)\n \t\tswitch (argc) {\n@@ -673,25 +677,24 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tcase 2: actions = ACTION_SET; break;\n \t\tcase 3: actions = ACTION_SET_ALL; break;\n \t\tdefault:\n-\t\t\tusage_with_options(builtin_config_usage, builtin_config_options);\n+\t\t\tusage_builtin_config();\n \t\t}\n \tif (omit_values &&\n \t    !(actions == ACTION_LIST || actions == ACTION_GET_REGEXP)) {\n \t\terror(\"--name-only is only applicable to --list or --get-regexp\");\n-\t\tusage_with_options(builtin_config_usage, builtin_config_options);\n+\t\tusage_builtin_config();\n \t}\n \n \tif (show_origin && !(actions &\n \t\t(ACTION_GET|ACTION_GET_ALL|ACTION_GET_REGEXP|ACTION_LIST))) {\n \t\terror(\"--show-origin is only applicable to --get, --get-all, \"\n \t\t\t  \"--get-regexp, and --list.\");\n-\t\tusage_with_options(builtin_config_usage, builtin_config_options);\n+\t\tusage_builtin_config();\n \t}\n \n \tif (default_value && !(actions & ACTION_GET)) {\n \t\terror(\"--default is only applicable to --get\");\n-\t\tusage_with_options(builtin_config_usage,\n-\t\t\tbuiltin_config_options);\n+\t\tusage_builtin_config();\n \t}\n \n \tif (actions & PAGING_ACTIONS)\n-- \n2.15.0.rc1.299.gda03b47c3\n\n"},{"id":"351713","messageId":"20180705193505.GA71957@syl.attlocal.net","threadId":"48831","inReplyTo":"20180705183445.30901-1-dev+git@drbeat.li","subject":"Re: [PATCH] builtin/config: work around an unsized array forward declaration","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-07-05T19:35:05Z","receivedAt":"2018-07-05T19:35:13Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Jul 05, 2018 at 08:34:45PM +0200, Beat Bolli wrote:\n> As reported here[0], Microsoft Visual Studio 2017.2 and \"gcc -pedantic\"\n> don't understand the forward declaration of an unsized static array.\n> They insist on an array size:\n>\n>     d:\\git\\src\\builtin\\config.c(70,46): error C2133: 'builtin_config_options': unknown size\n>\n> The thread [1] explains that this is due to the single-pass nature of\n> old compilers.\n>\n> To work around this error, introduce the forward-declared function\n> usage_builtin_config() instead that uses the array\n> builtin_config_options only after it has been defined.\n\nArgh, I think that this is my fault (via: fb0dc3bac1 (builtin/config.c:\nsupport `--type=<type>` as preferred alias for `--<type>`, 2018-04-18)).\n\nThank you for the explanation above, and for the patch below. I reviewed\nit myself, and the fix seems to be appropriate.\n\n> Also use this function in all other places where usage_with_options() is\n> called with the same arguments.\n>\n> [0]: https://github.com/git-for-windows/git/issues/1735\n> [1]: https://groups.google.com/forum/#!topic/comp.lang.c.moderated/bmiF2xMz51U\n>\n> Fixes https://github.com/git-for-windows/git/issues/1735\n>\n> Reported-By: Karen Huang (via GitHub)\n> Signed-off-by: Beat Bolli <dev+git@drbeat.li>\n\nThanks,\nTaylor\n"},{"id":"351714","messageId":"20180705193807.GA4826@sigill.intra.peff.net","threadId":"48831","inReplyTo":"20180705183445.30901-1-dev+git@drbeat.li","subject":"Re: [PATCH] builtin/config: work around an unsized array forward declaration","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-05T19:38:07Z","receivedAt":"2018-07-05T19:38:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 05, 2018 at 08:34:45PM +0200, Beat Bolli wrote:\n\n> As reported here[0], Microsoft Visual Studio 2017.2 and \"gcc -pedantic\"\n> don't understand the forward declaration of an unsized static array.\n> They insist on an array size:\n> \n>     d:\\git\\src\\builtin\\config.c(70,46): error C2133: 'builtin_config_options': unknown size\n> \n> The thread [1] explains that this is due to the single-pass nature of\n> old compilers.\n\nRight, that makes sense.\n\n> To work around this error, introduce the forward-declared function\n> usage_builtin_config() instead that uses the array\n> builtin_config_options only after it has been defined.\n> \n> Also use this function in all other places where usage_with_options() is\n> called with the same arguments.\n\nYour patch is obviously correct, but I think here there might be an even\nsimpler solution: just bump option_parse_type() below the declaration,\nsince it's the only one that needs it. That hunk is bigger, but the\noverall diff is simpler, and we don't need to carry that extra wrapper\nfunction.\n\nAs a general rule for this case (because reordering isn't always an\noption), I also wonder if we should prefer just introducing a pointer\nalias:\n\n  /* forward declaration is a pointer */\n  static struct option *builtin_config_options;\n\n  /* later, declare the actual storage and its alias */\n  static struct option builtin_config_options_storage[] = {\n\t...\n  };\n  static struct option *builtin_config_options = builtin_config_options_storage;\n\nThere are occasionally cases where the caller really wants an array and\nnot a pointer, but in practice those are pretty rare.\n\nI have a slight preference for the reordering solution in this case, but\nany of them would be OK with me.\n\n-Peff\n"},{"id":"351715","messageId":"phlsmp$mot$1@blaine.gmane.org","threadId":"48831","inReplyTo":"20180705193807.GA4826@sigill.intra.peff.net","subject":"Re: [PATCH] builtin/config: work around an unsized array forward declaration","fromName":"Beat Bolli","fromEmail":"dev+git@drbeat.li","sentAt":"2018-07-05T19:50:53Z","receivedAt":"2018-07-05T19:51:05Z","isPatch":true,"sender":{"key":"dev+git@drbeat.li","avatar":"https://avatars.githubusercontent.com/u/21444?v=4"},"body":"Hi Peff\n\nOn 05.07.18 21:38, Jeff King wrote:\n> On Thu, Jul 05, 2018 at 08:34:45PM +0200, Beat Bolli wrote:\n> \n>> As reported here[0], Microsoft Visual Studio 2017.2 and \"gcc -pedantic\"\n>> don't understand the forward declaration of an unsized static array.\n>> They insist on an array size:\n>>\n>>     d:\\git\\src\\builtin\\config.c(70,46): error C2133: 'builtin_config_options': unknown size\n>>\n>> The thread [1] explains that this is due to the single-pass nature of\n>> old compilers.\n> \n> Right, that makes sense.\n> \n>> To work around this error, introduce the forward-declared function\n>> usage_builtin_config() instead that uses the array\n>> builtin_config_options only after it has been defined.\n>>\n>> Also use this function in all other places where usage_with_options() is\n>> called with the same arguments.\n> \n> Your patch is obviously correct, but I think here there might be an even\n> simpler solution: just bump option_parse_type() below the declaration,\n> since it's the only one that needs it. That hunk is bigger, but the\n> overall diff is simpler, and we don't need to carry that extra wrapper\n> function.\n\nThat was dscho's first try in the GitHub issue. It doesn't compile\nbecause the OPT_CALLBACK* macros in the builtin_config_options\ndeclaration inserts a pointer to option_parse_type into the array items.\nWe need at least one forward declaration, and my patch seemed the least\nintrusive.\n\n> As a general rule for this case (because reordering isn't always an\n> option), I also wonder if we should prefer just introducing a pointer\n> alias:\n> \n>   /* forward declaration is a pointer */\n>   static struct option *builtin_config_options;\n> \n>   /* later, declare the actual storage and its alias */\n>   static struct option builtin_config_options_storage[] = {\n> \t...\n>   };\n>   static struct option *builtin_config_options = builtin_config_options_storage;\n> \n> There are occasionally cases where the caller really wants an array and\n> not a pointer, but in practice those are pretty rare.\n> \n> I have a slight preference for the reordering solution in this case, but\n> any of them would be OK with me.\n> \n> -Peff \n\nRegards, Beat\n\n"},{"id":"351716","messageId":"20180705200205.GA29861@sigill.intra.peff.net","threadId":"48831","inReplyTo":"phlsmp$mot$1@blaine.gmane.org","subject":"Re: [PATCH] builtin/config: work around an unsized array forward declaration","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-05T20:02:05Z","receivedAt":"2018-07-05T20:02:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 05, 2018 at 09:50:53PM +0200, Beat Bolli wrote:\n\n> > Your patch is obviously correct, but I think here there might be an even\n> > simpler solution: just bump option_parse_type() below the declaration,\n> > since it's the only one that needs it. That hunk is bigger, but the\n> > overall diff is simpler, and we don't need to carry that extra wrapper\n> > function.\n> \n> That was dscho's first try in the GitHub issue. It doesn't compile\n> because the OPT_CALLBACK* macros in the builtin_config_options\n> declaration inserts a pointer to option_parse_type into the array items.\n> We need at least one forward declaration, and my patch seemed the least\n> intrusive.\n\nAh, right, so it actually is mutually recursive.  Forward-declaring\noption_parse_type() would fix it, along with the reordering. I'm\nambivalent between the available options, then; we might as well go with\nwhat you posted, then, since it's already done. :)\n\n-Peff\n"},{"id":"351795","messageId":"xmqq1scgkw06.fsf@gitster-ct.c.googlers.com","threadId":"48831","inReplyTo":"20180705200205.GA29861@sigill.intra.peff.net","subject":"Re: [PATCH] builtin/config: work around an unsized array forward declaration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-06T19:24:41Z","receivedAt":"2018-07-06T19:24:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Thu, Jul 05, 2018 at 09:50:53PM +0200, Beat Bolli wrote:\n>\n>> > Your patch is obviously correct, but I think here there might be an even\n>> > simpler solution: just bump option_parse_type() below the declaration,\n>> > since it's the only one that needs it. That hunk is bigger, but the\n>> > overall diff is simpler, and we don't need to carry that extra wrapper\n>> > function.\n>> \n>> That was dscho's first try in the GitHub issue. It doesn't compile\n>> because the OPT_CALLBACK* macros in the builtin_config_options\n>> declaration inserts a pointer to option_parse_type into the array items.\n>> We need at least one forward declaration, and my patch seemed the least\n>> intrusive.\n>\n> Ah, right, so it actually is mutually recursive.  Forward-declaring\n> option_parse_type() would fix it, along with the reordering. I'm\n> ambivalent between the available options, then; we might as well go with\n> what you posted, then, since it's already done. :)\n\nAmong three, forward declaration of the function with reordering\nthat nobody has written except for in the brain smells the best, and\nturning an array to a pointer that points at a separate storage looked\nthe worst.  I also am OK with what's already posted, too.\n\nThanks.\n"},{"id":"351862","messageId":"20180707235825.GA1546@infogroep.be","threadId":"48831","inReplyTo":"xmqq1scgkw06.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] builtin/config: work around an unsized array forward declaration","fromName":"Kim Gybels","fromEmail":"kgybels@infogroep.be","sentAt":"2018-07-07T23:58:25Z","receivedAt":"2018-07-07T23:58:29Z","isPatch":true,"sender":{"key":"kgybels@infogroep.be","avatar":"https://avatars.githubusercontent.com/u/2051188?v=4"},"body":"On (06/07/18 12:24), Junio C Hamano wrote:\n> \n> Jeff King <peff@peff.net> writes:\n> \n> > On Thu, Jul 05, 2018 at 09:50:53PM +0200, Beat Bolli wrote:\n> >\n> >> > Your patch is obviously correct, but I think here there might be an even\n> >> > simpler solution: just bump option_parse_type() below the declaration,\n> >> > since it's the only one that needs it. That hunk is bigger, but the\n> >> > overall diff is simpler, and we don't need to carry that extra wrapper\n> >> > function.\n> >> \n> >> That was dscho's first try in the GitHub issue. It doesn't compile\n> >> because the OPT_CALLBACK* macros in the builtin_config_options\n> >> declaration inserts a pointer to option_parse_type into the array items.\n> >> We need at least one forward declaration, and my patch seemed the least\n> >> intrusive.\n> >\n> > Ah, right, so it actually is mutually recursive.  Forward-declaring\n> > option_parse_type() would fix it, along with the reordering. I'm\n> > ambivalent between the available options, then; we might as well go with\n> > what you posted, then, since it's already done. :)\n> \n> Among three, forward declaration of the function with reordering\n> that nobody has written except for in the brain smells the best, and\n> turning an array to a pointer that points at a separate storage looked\n> the worst.  I also am OK with what's already posted, too.\n\nI posted the forward declaration of the function on the Git for\nWindows issue:\nhttps://github.com/git-for-windows/git/issues/1735#issuecomment-402825623\n\nI would consider it the minimal fix. The already posted solution is\nalso OK for me.\n\nIt's also possible to use \"extern\" instead of \"static\" for the array.\nIt would, however, not be my preferred solution.\n\n-Kim\n"}]}