> Changelog libiberty/
> * pex-win32.c (argv_to_cmdline): Don't quote
> args unnecessarily
Some minor comments...
> > diff --git a/libiberty/pex-win32.c b/libiberty/pex-win32.c
> > index eae72c5..775b53c 100644
> > --- a/libiberty/pex-win32.c
> > +++ b/libiberty/pex-win32.c
> > @@ -340,17 +340,26 @@ argv_to_cmdline (char *const *argv)
> > char *p;
> > size_t cmdline_len;
> > int i, j, k;
> > + int needs_quotes;
> >
> > cmdline_len = 0;
> > for (i = 0; argv[i]; i++)
> > {
> > - /* We quote every last argument. This simplifies the problem;
> > - we need only escape embedded double-quotes and immediately
> > + /* We only quote arguments that contain spaces, \n \t \v or "
> > characters
> > + to prevent wasting 2 chars per argument of the CreateProcess 32k char
> > + limit We need only escape embedded double-quotes and immediately
Period missing after "limit". JIC, remember that we ask that periods be
followed by 2 spaces.
> > preceeding backslash characters. A sequence of backslach characters
> > that is not follwed by a double quote character will not be
> > escaped. */
> > + needs_quotes = 0;
> > for (j = 0; argv[i][j]; j++)
> > {
> > + if (argv[i][j] == ' ' || argv[i][j] == '\n' ||
> > + argv[i][j] == '\t' || argv[i][j] == '"' )
GNU Coding Style requirement: Binary operators should be at the start
of the next line, not at the end. For instance:
if (argv[i][j] == ' ' || argv[i][j] == '\n'
|| argv[i][j] == '\t' || argv[i][j] == '"')
Also, watch out for the extra space before ')' - please remove it.
> > + {
> Here seems to be an intend issue.
> > + needs_quotes = 1;
> > + }
> > +
> > if (argv[i][j] == '"')
> > {
> > /* Escape preceeding backslashes. */
> > @@ -362,16 +371,34 @@ argv_to_cmdline (char *const *argv)
> > }
> > /* Trailing backslashes also need to be escaped because they will be
> > followed by the terminating quote. */
> > - for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--)
> > - cmdline_len++;
> > + if (needs_quotes)
> > + {
> > + for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--)
> > + cmdline_len++;
> > + }
> > cmdline_len += j;
> > - cmdline_len += 3; /* for leading and trailing quotes and space */
> > + /* for leading and trailing quotes and space */
> > + cmdline_len += needs_quotes * 2 + 1;
> > }
> > cmdline = XNEWVEC (char, cmdline_len);
> > p = cmdline;
> > for (i = 0; argv[i]; i++)
> > {
> > - *p++ = '"';
> > + needs_quotes = 0;
> > + for (j = 0; argv[i][j]; j++)
> > + {
> > + if (argv[i][j] == ' ' || argv[i][j] == '\n' ||
> > + argv[i][j] == '\t' || argv[i][j] == '"' )
Same as above.
> > + {
> > + needs_quotes = 1;
> > + break;
> > + }
> > + }
> > +
> > + if (needs_quotes)
> > + {
> > + *p++ = '"';
> > + }
> > for (j = 0; argv[i][j]; j++)
> > {
> > if (argv[i][j] == '"')
> > @@ -382,9 +409,12 @@ argv_to_cmdline (char *const *argv)
> > }
> > *p++ = argv[i][j];
> > }
> > - for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--)
> > - *p++ = '\\';
> > - *p++ = '"';
> > + if (needs_quotes)
> > + {
> > + for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--)
> > + *p++ = '\\';
> > + *p++ = '"';
> > + }
> > *p++ = ' ';
> > }
> > p[-1] = '\0';
> > --
> > 1.9.2
>
> Patch itself makes sense. Let see if there are additional comments.
>
> Regards,
> Kai
--
Joel
