Re: [PATCH] Introduce skip_to_char and use it

[Sergio Durigan Junior <sergiodj at redhat dot com>]

On Tuesday, January 26 2016, Simon Marchi wrote:

> For a forthcoming patch, I need a "skip_to_colon" function.  I noticed
> there are two skip_to_semicolon (one in gdb and one in gdbserver).
>
> I thought we could put it in common/, and generalize it for any
> character.  To me,

Agreed.

>   p = skip_to_char (p, ';');
>
> is at least as clear as
>
>   p = skip_to_semicolon (p);
>
> so I decided to just remove skip_to_semicolon and use skip_to_char.

The problem with skip_to_char IMO is that it leads the reader to believe
that this function performs the same job as strchr (in fact, my first
reaction when I read this introduction was to think "why not use
strchr?").  The main difference, however, is that this function actually
skips everything if the char was not found.

I believe a name like "skip_to_char_or_nul" may be more appropriate.
What do you think?

FWIW, I know that skip_to_space suffers from this same problem, so if
you like this idea and others approve I can send a patch renaming it to
skip_to_space_or_nul.

Other than that, the patch looks good to me.

> gdb/ChangeLog:
>
> 	* common/common-utils.h (skip_to_char): New declaration.
> 	* common/common-utils.c (skip_to_char): New function.
> 	* remote.c (skip_to_semicolon): Remove.
> 	(remote_parse_stop_reply): Use skip_to_char instead of
> 	skip_to_semicolon.
>
> gdb/gdbserver/ChangeLog:
>
> 	* server.c (skip_to_semicolon):  Remove.
> 	(process_point_options): Use skip_to_char instead of
> 	skip_to_semicolon.
> ---
>  gdb/common/common-utils.c | 12 ++++++++++++
>  gdb/common/common-utils.h |  5 +++++
>  gdb/gdbserver/server.c    | 15 +++------------
>  gdb/remote.c              | 26 ++++++++------------------
>  4 files changed, 28 insertions(+), 30 deletions(-)
>
> diff --git a/gdb/common/common-utils.c b/gdb/common/common-utils.c
> index 33668f3..32720b2 100644
> --- a/gdb/common/common-utils.c
> +++ b/gdb/common/common-utils.c
> @@ -288,3 +288,15 @@ skip_to_space_const (const char *chp)
>      chp++;
>    return chp;
>  }
> +
> +char *
> +skip_to_char (char *chp, char to)
> +{
> +  if (chp == NULL)
> +    return NULL;
> +
> +  while (*chp != '\0' && *chp != to)
> +    chp++;
> +
> +  return chp;
> +}
> diff --git a/gdb/common/common-utils.h b/gdb/common/common-utils.h
> index 47def11..67b9df7 100644
> --- a/gdb/common/common-utils.h
> +++ b/gdb/common/common-utils.h
> @@ -97,4 +97,9 @@ extern const char *skip_spaces_const (const char *inp);
>  
>  extern const char *skip_to_space_const (const char *inp);
>  
> +/* Skip characters until CHP points to a character with value TO, or the end of
> +   the string.  Return the updated pointer.  If CHP is NULL, return NULL.  */
> +
> +extern char *skip_to_char (char *chp, char to);
> +
>  #endif
> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
> index 301dea9..6d70ee7 100644
> --- a/gdb/gdbserver/server.c
> +++ b/gdb/gdbserver/server.c
> @@ -3820,15 +3820,6 @@ main (int argc, char *argv[])
>    gdb_assert_not_reached ("captured_main should never return");
>  }
>  
> -/* Skip PACKET until the next semi-colon (or end of string).  */
> -
> -static void
> -skip_to_semicolon (char **packet)
> -{
> -  while (**packet != '\0' && **packet != ';')
> -    (*packet)++;
> -}
> -
>  /* Process options coming from Z packets for a breakpoint.  PACKET is
>     the packet buffer.  *PACKET is updated to point to the first char
>     after the last processed option.  */
> @@ -3856,7 +3847,7 @@ process_point_options (struct breakpoint *bp, char **packet)
>  	  if (debug_threads)
>  	    debug_printf ("Found breakpoint condition.\n");
>  	  if (!add_breakpoint_condition (bp, &dataptr))
> -	    skip_to_semicolon (&dataptr);
> +	    dataptr = skip_to_char (dataptr, ';');
>  	}
>        else if (startswith (dataptr, "cmds:"))
>  	{
> @@ -3866,14 +3857,14 @@ process_point_options (struct breakpoint *bp, char **packet)
>  	  persist = (*dataptr == '1');
>  	  dataptr += 2;
>  	  if (add_breakpoint_commands (bp, &dataptr, persist))
> -	    skip_to_semicolon (&dataptr);
> +	    dataptr = skip_to_char (dataptr, ';');
>  	}
>        else
>  	{
>  	  fprintf (stderr, "Unknown token %c, ignoring.\n",
>  		   *dataptr);
>  	  /* Skip tokens until we find one that we recognize.  */
> -	  skip_to_semicolon (&dataptr);
> +	  dataptr = skip_to_char (dataptr, ';');
>  	}
>      }
>    *packet = dataptr;
> diff --git a/gdb/remote.c b/gdb/remote.c
> index b0303f6..a65a384 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -6387,16 +6387,6 @@ peek_stop_reply (ptid_t ptid)
>  			 stop_reply_match_ptid_and_ws, &ptid);
>  }
>  
> -/* Skip PACKET until the next semi-colon (or end of string).  */
> -
> -static char *
> -skip_to_semicolon (char *p)
> -{
> -  while (*p != '\0' && *p != ';')
> -    p++;
> -  return p;
> -}
> -
>  /* Helper for remote_parse_stop_reply.  Return nonzero if the substring
>     starting with P and ending with PEND matches PREFIX.  */
>  
> @@ -6499,7 +6489,7 @@ Packet: '%s'\n"),
>  	      /* The value part is documented as "must be empty",
>  		 though we ignore it, in case we ever decide to make
>  		 use of it in a backward compatible way.  */
> -	      p = skip_to_semicolon (p1 + 1);
> +	      p = skip_to_char (p1 + 1, ';');
>  	    }
>  	  else if (strprefix (p, p1, "hwbreak"))
>  	    {
> @@ -6511,19 +6501,19 @@ Packet: '%s'\n"),
>  		error (_("Unexpected hwbreak stop reason"));
>  
>  	      /* See above.  */
> -	      p = skip_to_semicolon (p1 + 1);
> +	      p = skip_to_char (p1 + 1, ';');
>  	    }
>  	  else if (strprefix (p, p1, "library"))
>  	    {
>  	      event->ws.kind = TARGET_WAITKIND_LOADED;
> -	      p = skip_to_semicolon (p1 + 1);
> +	      p = skip_to_char (p1 + 1, ';');
>  	    }
>  	  else if (strprefix (p, p1, "replaylog"))
>  	    {
>  	      event->ws.kind = TARGET_WAITKIND_NO_HISTORY;
>  	      /* p1 will indicate "begin" or "end", but it makes
>  		 no difference for now, so ignore it.  */
> -	      p = skip_to_semicolon (p1 + 1);
> +	      p = skip_to_char (p1 + 1, ';');
>  	    }
>  	  else if (strprefix (p, p1, "core"))
>  	    {
> @@ -6545,7 +6535,7 @@ Packet: '%s'\n"),
>  	  else if (strprefix (p, p1, "vforkdone"))
>  	    {
>  	      event->ws.kind = TARGET_WAITKIND_VFORK_DONE;
> -	      p = skip_to_semicolon (p1 + 1);
> +	      p = skip_to_char (p1 + 1, ';');
>  	    }
>  	  else if (strprefix (p, p1, "exec"))
>  	    {
> @@ -6574,7 +6564,7 @@ Packet: '%s'\n"),
>  	  else if (strprefix (p, p1, "create"))
>  	    {
>  	      event->ws.kind = TARGET_WAITKIND_THREAD_CREATED;
> -	      p = skip_to_semicolon (p1 + 1);
> +	      p = skip_to_char (p1 + 1, ';');
>  	    }
>  	  else
>  	    {
> @@ -6583,7 +6573,7 @@ Packet: '%s'\n"),
>  
>  	      if (skipregs)
>  		{
> -		  p = skip_to_semicolon (p1 + 1);
> +		  p = skip_to_char (p1 + 1, ';');
>  		  p++;
>  		  continue;
>  		}
> @@ -6620,7 +6610,7 @@ Packet: '%s'\n"),
>  		{
>  		  /* Not a number.  Silently skip unknown optional
>  		     info.  */
> -		  p = skip_to_semicolon (p1 + 1);
> +		  p = skip_to_char (p1 + 1, ';');
>  		}
>  	    }
>  
> -- 
> 2.5.1

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/