Re: [PATCH v3 2/6] gdbsupport: Adapt construct_inferior_arguments

Simon Marchi
2020-05-12T17:53:45Z

Just some nits.

On 2020-05-12 11:42 a.m., Michael Weghorn via Gdb-patches wrote:
> diff --git a/gdbsupport/common-inferior.cc b/gdbsupport/common-inferior.cc
> index a7d631f357..cadbd50e9c 100644
> --- a/gdbsupport/common-inferior.cc
> +++ b/gdbsupport/common-inferior.cc
> @@ -27,15 +27,12 @@ bool startup_with_shell = true;
>
> /* See common-inferior.h. */
>
> -char *
> -construct_inferior_arguments (int argc, char **argv)
> +std::string
> +construct_inferior_arguments (int argc, char * const *argv)
> {
> - char *result;
> + gdb_assert (argc >= 0);
>
> - /* ARGC should always be at least 1, but we double check this
> - here. This is also needed to silence -Werror-stringop
> - warnings. */
> - gdb_assert (argc > 0);
> + std::string result;
>
> if (startup_with_shell)
> {
> @@ -52,48 +49,39 @@ construct_inferior_arguments (int argc, char **argv)
> static const char quote = '\'';
> #endif
> int i;

Can you inline this in the for loop while at it?

> - int length = 0;
> - char *out, *cp;
> -
> - /* We over-compute the size. It shouldn't matter. */
> - for (i = 0; i < argc; ++i)
> - length += 3 * strlen (argv[i]) + 1 + 2 * (argv[i][0] == '\0');
> -
> - result = (char *) xmalloc (length);
> - out = result;
>
> for (i = 0; i < argc; ++i)
> {
> if (i > 0)
> - *out++ = ' ';
> + result += ' ';
>
> /* Need to handle empty arguments specially. */
> if (argv[i][0] == '\0')
> {
> - *out++ = quote;
> - *out++ = quote;
> + result += quote;
> + result += quote;
> }
> else
> {
> #ifdef __MINGW32__
> - int quoted = 0;
> + bool quoted = 0;

Replace 0 with false.

>
> if (strpbrk (argv[i], special))
> {
> - quoted = 1;
> - *out++ = quote;
> + quoted = true;
> + result += quote;
> }
> #endif
> - for (cp = argv[i]; *cp; ++cp)
> + for (char *cp = argv[i]; *cp; ++cp)
> {
> if (*cp == '\n')
> {
> /* A newline cannot be quoted with a backslash (it
> just disappears), only by putting it inside
> quotes. */
> - *out++ = quote;
> - *out++ = '\n';
> - *out++ = quote;
> + result += quote;
> + result += '\n';
> + result += quote;
> }
> else
> {
> @@ -102,24 +90,22 @@ construct_inferior_arguments (int argc, char **argv)
> #else
> if (strchr (special, *cp) != NULL)
> #endif
> - *out++ = '\\';
> - *out++ = *cp;
> + result += '\\';
> + result += *cp;
> }
> }
> #ifdef __MINGW32__
> if (quoted)
> - *out++ = quote;
> + result += quote;
> #endif
> }
> }
> - *out = '\0';
> }
> else
> {
> /* In this case we can't handle arguments that contain spaces,
> tabs, or newlines -- see breakup_args(). */
> int i;

Can you also inline this one in the for loops?

Simon