Re: [PATCH] fixincludes: fix fixincludes for MinGW

[Jeff Law <law at redhat dot com>]

On 09/22/2016 11:26 PM, Tadek Kijkowski wrote:
> The fixincl executable uses system function to call applyfix or to
> direcly patch a header file, with parameters enclosed in single
> quotes. This problem is that MinGW system function just calls cmd.exe,
> which doesn't strip quotes from parameters and completely ignores
> quotes for embedding spaces in parameters. The MinGW system function
> also doesn't allow for newlines in executed command parameters. As a
> result fixincludes doesn't wotk at on when trying to build a cross
> compiler with mingw as host.
>
> On MinGW host, this patch adds system_with_shell function, which
> transforms command passed to system function is following way:
> - Enclose entire command in double quotes
> - Prepend shell executable name, taken from environment variable SHELL
> - Replace each occurence of newline with '$'\n'' sequence which is
> understood by bash and ksh (it is assumed that all newlines are
> embedded in command parameters enclosed in single quotes)
>
> 2016-09-23 Tadek Kijkowski (tkijkowski@gmail.com)
>
>         * fixincl.c: Added system_with_shell for MinGW host.
>
>
> Index: fixincludes/fixincl.c
> ===================================================================
> --- fixincludes/fixincl.c   (revision 240386)
> +++ fixincludes/fixincl.c   (working copy)
> @@ -170,7 +170,111 @@
>    exit (EXIT_SUCCESS);
>  }
>
> +#ifdef __MINGW32__
>
> +/* Count number of \c needle character instances in string */
> +static int
> +count_chars ( char* str, char needle )
Formatting it.  This should be:

count_chars (char* str, char needle)

Note the lack of horizontal whitespace after the open paren or before 
the close paren.  Similarly for system_with_shell declaration below.

Wouldn't this be better named "count_occurrences_of_char" or 
"count_instances_of_char"?




> +
> +  /* Allocate enough memory to fit newly created command string */
> +  cmd_size = strlen (env_shell) + (sizeof (z_shell_start_args) - 1)
> +           + strlen (s) + newline_cnt * (sizeof(z_shell_newline) - 1 - 1)
> +           + (sizeof (z_shell_end_args) - 1) + 1;
Why use sizeof (foo) - 1 rather than just strlen (foo) here?  Note that 
GCC can compute the string length as a compile time constant, so you're 
not gaining any performance by using sizeof here and strlen seems clearer.



> +
> +  long_cmd = XNEWVEC (char, cmd_size);
> +
> +  /* Start with ${SHELL} -c " */
> +  strcpy (long_cmd, env_shell);
> +  strcat (long_cmd, z_shell_start_args);
> +
> +  /* End pointer for appending pieces of command while replacing newlines */
> +  cmd_endp = long_cmd + strlen(long_cmd);
Formatting nit.  Space between function name and its argument list.


> +  nl_scan = s;
> +  while (nl_scan != NULL)
> +    {
> +      char* next_nl = strchr (nl_scan, '\n');
Formatting nit.  char *next_nl.


> +      if (next_nl)
> +        {
> +          /* Append part of command between newlines */
> +          size_t part_len = next_nl - nl_scan;
> +          memcpy(cmd_endp, nl_scan, part_len);
Formatting nit, space between function name and its argument list.

> +          cmd_endp += part_len;
> +
> +          /* Append newline replacement */
> +          memcpy(cmd_endp, z_shell_newline, sizeof(z_shell_newline));
Smilarly, space between functio nname and its argument list.

> +          cmd_endp += sizeof(z_shell_newline) - 1;
Here too.

> +
> +  free ( (void*) long_cmd);
free (long_cmd);

Can you fix those various (minor) issue and resubmit.  I think it'll be 
OK for the trunk with those changes.

jeff