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

Pedro Alves
2016-10-19T21:45:01Z

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