builtin/config: work around an unsized array forward declaration

As reported here[0], Microsoft Visual Studio 2017.2 and "gcc -pedantic" don't understand the forward declaration of an unsized static array. They insist on an array size:

    d:\git\src\builtin\config.c(70,46): error C2133: 'builtin_config_options': unknown size

The thread [1] explains that this is due to the single-pass nature of old compilers.

To work around this error, introduce the forward-declared function usage_builtin_config() instead that uses the array builtin_config_options only after it has been defined.

Also use this function in all other places where usage_with_options() is called with the same arguments.

[0]: https://github.com/git-for-windows/git/issues/1735
[1]: https://groups.google.com/forum/#!topic/comp.lang.c.moderated/bmiF2xMz51U

Fixes https://github.com/git-for-windows/git/issues/1735

Reported-By Karen Huang (via GitHub)

Signed-off-by Beat Bolli <[email protected]>

As reported here[0], Microsoft Visual Studio 2017.2 and "gcc -pedantic" don't understand the forward declaration of an unsized static array. They insist on an array size:

    d:\git\src\builtin\config.c(70,46): error C2133: 'builtin_config_options': unknown size

The thread [1] explains that this is due to the single-pass nature of old compilers.

To work around this error, introduce the forward-declared function usage_builtin_config() instead that uses the array builtin_config_options only after it has been defined.

Also use this function in all other places where usage_with_options() is called with the same arguments.

Quoted 8 lines · Beat Bolli

As reported here[0], Microsoft Visual Studio 2017.2 and "gcc -pedantic" don't understand the forward declaration of an unsized static array. They insist on an array size:

    d:\git\src\builtin\config.c(70,46): error C2133: 'builtin_config_options': unknown size

The thread [1] explains that this is due to the single-pass nature of old compilers.

Right, that makes sense.

Quoted 4 lines

To work around this error, introduce the forward-declared function usage_builtin_config() instead that uses the array builtin_config_options only after it has been defined.

Quoted 2 lines

Also use this function in all other places where usage_with_options() is called with the same arguments.

Your patch is obviously correct, but I think here there might be an even simpler solution: just bump option_parse_type() below the declaration, since it's the only one that needs it. That hunk is bigger, but the overall diff is simpler, and we don't need to carry that extra wrapper function.

As a general rule for this case (because reordering isn't always an option), I also wonder if we should prefer just introducing a pointer alias:

/* forward declaration is a pointer */
static struct option *builtin_config_options;

/* later, declare the actual storage and its alias */

  static struct option builtin_config_options_storage[] = {
	...

};
static struct option *builtin_config_options = builtin_config_options_storage;

There are occasionally cases where the caller really wants an array and not a pointer, but in practice those are pretty rare.

I have a slight preference for the reordering solution in this case, but any of them would be OK with me.

-Peff

Quoted 5 lines

Your patch is obviously correct, but I think here there might be an even simpler solution: just bump option_parse_type() below the declaration, since it's the only one that needs it. That hunk is bigger, but the overall diff is simpler, and we don't need to carry that extra wrapper function.

That was dscho's first try in the GitHub issue. It doesn't compile because the OPT_CALLBACK* macros in the builtin_config_options declaration inserts a pointer to option_parse_type into the array items. We need at least one forward declaration, and my patch seemed the least intrusive.

Quoted 11 lines · Beat Bolli

Quoted 5 lines

Your patch is obviously correct, but I think here there might be an even simpler solution: just bump option_parse_type() below the declaration, since it's the only one that needs it. That hunk is bigger, but the overall diff is simpler, and we don't need to carry that extra wrapper function.

That was dscho's first try in the GitHub issue. It doesn't compile because the OPT_CALLBACK* macros in the builtin_config_options declaration inserts a pointer to option_parse_type into the array items. We need at least one forward declaration, and my patch seemed the least intrusive.

Ah, right, so it actually is mutually recursive. Forward-declaring option_parse_type() would fix it, along with the reordering. I'm ambivalent between the available options, then; we might as well go with what you posted, then, since it's already done. :)

Quoted 14 lines · Jeff King

Quoted 11 lines · Beat Bolli

Quoted 5 lines

Your patch is obviously correct, but I think here there might be an even simpler solution: just bump option_parse_type() below the declaration, since it's the only one that needs it. That hunk is bigger, but the overall diff is simpler, and we don't need to carry that extra wrapper function.

That was dscho's first try in the GitHub issue. It doesn't compile because the OPT_CALLBACK* macros in the builtin_config_options declaration inserts a pointer to option_parse_type into the array items. We need at least one forward declaration, and my patch seemed the least intrusive.

Quoted 4 lines

Ah, right, so it actually is mutually recursive. Forward-declaring option_parse_type() would fix it, along with the reordering. I'm ambivalent between the available options, then; we might as well go with what you posted, then, since it's already done. :)

Among three, forward declaration of the function with reordering that nobody has written except for in the brain smells the best, and turning an array to a pointer that points at a separate storage looked the worst. I also am OK with what's already posted, too.