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

Simon Marchi
2016-10-19T18:25:47Z

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

Do you know of any caller of execute_command that relies on the
modification that execute_command does to the passed string? If not, I
think it would be safer to change the arg to a const char * and
duplicate the string at function entry.