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

Michael Weghorn
2020-05-13T09:56:12Z

On 12/05/2020 19.53, Simon Marchi wrote:
> 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?

Yes, done in v4.

>
>> - 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.

Done in v4.
>
>>
>> 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?

Yes done in v4.

Michael