Re: [PATCH v2 04/31] cli-script.c: Simplify using std::string, eliminate cleanups

[Pedro Alves <palves@redhat.com>]

On 10/19/2016 07:25 PM, Simon Marchi wrote:
> On 2016-10-18 21:11, Pedro Alves wrote:
>> @@ -457,12 +456,13 @@ execute_control_command (struct command_line *cmd)
>>    switch (cmd->control_type)
>>      {
>>      case simple_control:
>> -      /* A simple command, execute it and return.  */
>> -      new_line = insert_args (cmd->line);
>> -      make_cleanup (free_current_contents, &new_line);
>> -      execute_command (new_line, 0);
>> -      ret = cmd->control_type;
>> -      break;
>> +      {
>> +    /* A simple command, execute it and return.  */
>> +    std::string new_line = insert_args (cmd->line);
>> +    execute_command (&new_line[0], 0);
> 
> The need to use &new_line[0] instead of .c_str() here looks (or smells)
> like a code smell to me.  

See my comment to the string_printf patch.  &str[0] is required to
return a pointer to modifiable contents.  What's not technically
defined is assuming that that returns a NULL-terminated buffer.
But in practice it does, everywhere.  See.  The second answer
describes it fully:

  http://stackoverflow.com/questions/347949/how-to-convert-a-stdstring-to-const-char-or-char

Writing a NULL to the middle of the string via the char pointer
won't change the std::string's size(), but that's not a problem
here, because new_line is a temporary and we don't care about
the new_line.size() after the execute_command call.

> If execute_command needs to modify the string,
> it should either make its own copy, or at least document in what ways it
> can be modified.  And in general, modifying an std::string's underlying
> array is probably not good (if I understand the standard correctly, it's
> undefined behaviour, though in reality it probably always works).

See above.

> Do you know of any caller of execute_command that relies on the
> modification that execute_command does to the passed string?  

The main execute_command call coming from the tty.  :-)

Some commands hack the command line to influence what's
actually repeated with <ret>.  Off hand, I remember list_command:

  /* If this command is repeated with RET,
     turn it into the no-arg variant.  */

  if (from_tty)
    *arg = 0;

> If not, I
> think it would be safer to change the arg to a const char * and
> duplicate the string at function entry.

Thanks,
Pedro Alves