diff mbox

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

Message ID 1476839539-8374-5-git-send-email-palves@redhat.com
State New, archived
Headers

Commit Message

Pedro Alves Oct. 19, 2016, 1:11 a.m. UTC 
  gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* cli/cli-script.c (execute_control_command): Use std::string
	instead of cleanups.
	(locate_arg): Constify return type.
	(insert_args): Constify paremeter.  Simplify using std::string.
	Return a std::string.
---
 gdb/cli/cli-script.c | 110 ++++++++++++++++-----------------------------------
 1 file changed, 33 insertions(+), 77 deletions(-)

Comments

Simon Marchi Oct. 19, 2016, 6:25 p.m. UTC | #1 
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.
Pedro Alves Oct. 19, 2016, 9:44 p.m. UTC | #2 
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
diff mbox

Patch

diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c
index c36cce6..6f1cc8a 100644
--- a/gdb/cli/cli-script.c
+++ b/gdb/cli/cli-script.c
@@ -41,7 +41,7 @@  recurse_read_control_structure (char * (*read_next_line_func) (void),
 				void (*validator)(char *, void *),
 				void *closure);
 
-static char *insert_args (char *line);
+static std::string insert_args (const char *line);
 
 static struct cleanup * setup_user_args (char *p);
 
@@ -448,7 +448,6 @@  execute_control_command (struct command_line *cmd)
   struct value *val_mark;
   int loop;
   enum command_control_type ret;
-  char *new_line;
 
   /* Start by assuming failure, if a problem is detected, the code
      below will simply "break" out of the switch.  */
@@ -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);
+	ret = cmd->control_type;
+	break;
+      }
 
     case continue_control:
       print_command_trace ("loop_continue");
@@ -489,9 +489,8 @@  execute_control_command (struct command_line *cmd)
 	print_command_trace (buffer);
 
 	/* Parse the loop control expression for the while statement.  */
-	new_line = insert_args (cmd->line);
-	make_cleanup (free_current_contents, &new_line);
-	expr = parse_expression (new_line);
+	std::string new_line = insert_args (cmd->line);
+	expr = parse_expression (new_line.c_str ());
 	make_cleanup (free_current_contents, &expr);
 
 	ret = simple_control;
@@ -555,10 +554,9 @@  execute_control_command (struct command_line *cmd)
 	xsnprintf (buffer, len, "if %s", cmd->line);
 	print_command_trace (buffer);
 
-	new_line = insert_args (cmd->line);
-	make_cleanup (free_current_contents, &new_line);
 	/* Parse the conditional for the if statement.  */
-	expr = parse_expression (new_line);
+	std::string new_line = insert_args (cmd->line);
+	expr = parse_expression (new_line.c_str ());
 	make_cleanup (free_current_contents, &expr);
 
 	current = NULL;
@@ -598,9 +596,8 @@  execute_control_command (struct command_line *cmd)
       {
 	/* Breakpoint commands list, record the commands in the
 	   breakpoint's command list and return.  */
-	new_line = insert_args (cmd->line);
-	make_cleanup (free_current_contents, &new_line);
-	ret = commands_from_control_command (new_line, cmd);
+	std::string new_line = insert_args (cmd->line);
+	ret = commands_from_control_command (new_line.c_str (), cmd);
 	break;
       }
 
@@ -786,8 +783,8 @@  setup_user_args (char *p)
 /* Given character string P, return a point to the first argument
    ($arg), or NULL if P contains no arguments.  */
 
-static char *
-locate_arg (char *p)
+static const char *
+locate_arg (const char *p)
 {
   while ((p = strchr (p, '$')))
     {
@@ -800,93 +797,52 @@  locate_arg (char *p)
 }
 
 /* Insert the user defined arguments stored in user_arg into the $arg
-   arguments found in line, with the updated copy being placed into
-   nline.  */
+   arguments found in line.  */
 
-static char *
-insert_args (char *line)
+static std::string
+insert_args (const char *line)
 {
-  char *p, *save_line, *new_line;
-  unsigned len, i;
-
   /* If we are not in a user-defined function, treat $argc, $arg0, et
      cetera as normal convenience variables.  */
   if (user_args == NULL)
-    return xstrdup (line);
+    return line;
 
-  /* First we need to know how much memory to allocate for the new
-     line.  */
-  save_line = line;
-  len = 0;
-  while ((p = locate_arg (line)))
-    {
-      len += p - line;
-      i = p[4] - '0';
-
-      if (p[4] == 'c')
-	{
-	  /* $argc.  Number will be <=10.  */
-	  len += user_args->count == 10 ? 2 : 1;
-	}
-      else if (i >= user_args->count)
-	{
-	  error (_("Missing argument %d in user function."), i);
-	  return NULL;
-	}
-      else
-	{
-	  len += user_args->a[i].len;
-	}
-      line = p + 5;
-    }
-
-  /* Don't forget the tail.  */
-  len += strlen (line);
-
-  /* Allocate space for the new line and fill it in.  */
-  new_line = (char *) xmalloc (len + 1);
-
-  /* Restore pointer to beginning of old line.  */
-  line = save_line;
-
-  /* Save pointer to beginning of new line.  */
-  save_line = new_line;
+  std::string new_line;
+  const char *p;
 
   while ((p = locate_arg (line)))
     {
       int i, len;
 
-      memcpy (new_line, line, p - line);
-      new_line += p - line;
+      new_line.append (line, p - line);
 
       if (p[4] == 'c')
 	{
 	  gdb_assert (user_args->count >= 0 && user_args->count <= 10);
 	  if (user_args->count == 10)
 	    {
-	      *(new_line++) = '1';
-	      *(new_line++) = '0';
+	      new_line += '1';
+	      new_line += '0';
 	    }
 	  else
-	    *(new_line++) = user_args->count + '0';
+	    new_line += user_args->count + '0';
 	}
       else
 	{
 	  i = p[4] - '0';
+	  if (i >= user_args->count)
+	    error (_("Missing argument %d in user function."), i);
+
 	  len = user_args->a[i].len;
-	  if (len)
-	    {
-	      memcpy (new_line, user_args->a[i].arg, len);
-	      new_line += len;
-	    }
+	  if (len > 0)
+	    new_line.append (user_args->a[i].arg, len);
 	}
       line = p + 5;
     }
   /* Don't forget the tail.  */
-  strcpy (new_line, line);
+  new_line.append (line);
 
-  /* Return a pointer to the beginning of the new line.  */
-  return save_line;
+  return new_line;
 }