Re: [PATCH v2 1/4] gdbsupport: Extend construct_inferior_arguments to allow handling all stringify_args cases

Simon Marchi
2020-05-07T19:49:08Z

Hi Michael,

I think the patch looks good, there are just a few nits to fix, and a suggestion.

> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index 8f7482347c..3ac64b3164 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -151,12 +151,9 @@ get_inferior_args (void)
> {
> if (current_inferior ()->argc != 0)
> {
> - char *n;
> -
> - n = construct_inferior_arguments (current_inferior ()->argc,
> + std::string n = construct_inferior_arguments (current_inferior ()->argc,
> current_inferior ()->argv);

Align the last line with the parenthesis, like it was before the change.

> - set_inferior_args (n);
> - xfree (n);
> + set_inferior_args (n.c_str());

Space before parenthesis.

> }
>
> if (current_inferior ()->args == NULL)
> diff --git a/gdbsupport/common-inferior.cc b/gdbsupport/common-inferior.cc
> index 71b9a11e02..3f117d5ef0 100644
> --- a/gdbsupport/common-inferior.cc
> +++ b/gdbsupport/common-inferior.cc
> @@ -28,15 +28,15 @@ bool startup_with_shell = true;
> /* Compute command-line string given argument vector. This does the
> same shell processing as fork_inferior. */
>
> -char *
> -construct_inferior_arguments (int argc, char **argv)
> +std::string
> +construct_inferior_arguments (int argc, char * const *argv)
> {
> - char *result;
> + gdb_assert (argc >= 0);
> + if (argc == 0 || argv[0] == NULL) {
> + return "";
> + }

Omit the braces when there is just one single line statement.

When there are braces, places them like this:

if (condition)
{
stmt1;
stmt2;
}

Did you actually encounter the case (argc > 0 && argv[0] == NULL)? In
other words, does it really constitute a valid input to this function?

>
> - /* 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);
> + char *result;
>
> if (startup_with_shell)
> {
> @@ -145,5 +145,8 @@ construct_inferior_arguments (int argc, char **argv)
> }
> }
>
> - return result;
> + std::string str_result(result);

Space before parenthesis.

If we are going to return a string anyway, I'd vote for building the result
into an std::string directly. You can remove the part that tries to guess
the length of the string, and use the appropriate std::string methods /
operators to append to it.

If you do this, I'm thinking that the `argc == 0` check at the beginning of
the function might be unnecessary, as the code could handle it naturally:
all the loops will be skipped and we'll return an empty string.

Thanks,

Simon