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

[Simon Marchi <simon dot marchi at polymtl dot ca>]

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.