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

[Michael Weghorn <m.weghorn@posteo.de>]

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