This is the mail archive of the mailing list for the GDB project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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

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.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]