Introduce skip_to_char and use it
Commit Message
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,
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.
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(-)
Comments
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
On 01/27/2016 01:32 AM, Sergio Durigan Junior wrote:
> 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?
That sounds just like strchrnul, though. It's not available everywhere,
but, gnulib has a strchrnul module that fixes that.
Thanks,
Pedro Alves
Pedro Alves <palves@redhat.com> writes:
> On 01/27/2016 01:32 AM, Sergio Durigan Junior wrote:
>
>> 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?
>
> That sounds just like strchrnul, though.
Or strcspn.
Andreas.
On Wednesday, January 27 2016, Pedro Alves wrote:
> On 01/27/2016 01:32 AM, Sergio Durigan Junior wrote:
>
>> 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?
>
> That sounds just like strchrnul, though. It's not available everywhere,
> but, gnulib has a strchrnul module that fixes that.
Even better. Thank you, Pedro.
On 16-01-27 06:21 AM, Pedro Alves wrote:
> That sounds just like strchrnul, though. It's not available everywhere,
> but, gnulib has a strchrnul module that fixes that.
Sounds good, I'll prepare a patch that uses that instead.
Thanks,
Simon
On 16-01-27 01:27 PM, Simon Marchi wrote:
> On 16-01-27 06:21 AM, Pedro Alves wrote:
>> That sounds just like strchrnul, though. It's not available everywhere,
>> but, gnulib has a strchrnul module that fixes that.
>
> Sounds good, I'll prepare a patch that uses that instead.
>
> Thanks,
>
> Simon
Here is the patch: https://sourceware.org/ml/gdb-patches/2016-01/msg00678.html
@@ -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;
+}
@@ -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
@@ -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;
@@ -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, ';');
}
}