This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Use strtok_r instead of strsep in rust_get_disr_info
- From: Manish Goregaokar <manish at mozilla dot com>
- To: Pedro Alves <palves at redhat dot com>, gdb-patches at sourceware dot org
- Cc: Tom Tromey <tom at tromey dot com>
- Date: Wed, 29 Jun 2016 16:05:39 +0530
- Subject: Re: [PATCH] Use strtok_r instead of strsep in rust_get_disr_info
- Authentication-results: sourceware.org; auth=none
- References: <CAFOnWkmN-uBB8iDjMaLpe=0SxWecFXxvvgfG2-FJn3j2XbBZJA at mail dot gmail dot com> <e349e31f-74b5-a2fa-b82b-5cbb6d548f16 at redhat dot com> <CAFOnWk=a2g-XEtsFjXp85DhTSmT4fVNSkWLuAa0WoS9JY_XM=w at mail dot gmail dot com>
Actually, it occurs to me that token could still be null after the
loop, and we use the value of token to get the name.
New patch:
--------------
strsep doesn't exist on Windows.
2016-06-29 Manish Goregaokar <manish@mozilla.com>
gdb/ChangeLog:
* rust-lang.c (rust_get_disr_info): Use strtok_r instead of strsep.
---
gdb/rust-lang.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/gdb/rust-lang.c b/gdb/rust-lang.c
index 23ddd5a..c01687a 100644
--- a/gdb/rust-lang.c
+++ b/gdb/rust-lang.c
@@ -121,7 +121,7 @@ rust_get_disr_info (struct type *type, const
gdb_byte *valaddr,
if (strncmp (TYPE_FIELD_NAME (type, 0), RUST_ENUM_PREFIX,
strlen (RUST_ENUM_PREFIX)) == 0)
{
- char *name, *tail, *token;
+ char *tail, *token, *name, *saveptr;
unsigned long fieldno;
struct type *member_type;
LONGEST value;
@@ -145,7 +145,9 @@ rust_get_disr_info (struct type *type, const
gdb_byte *valaddr,
traversed in order to find the field (which may be several
fields deep)
and the variantname is the name of the variant of the case when the
field is zero. */
- while ((token = strsep (&tail, "$")) != NULL)
+ for (token = strtok_r (tail, "$", &saveptr);
+ token != NULL;
+ token = strtok_r (NULL, "$", &saveptr))
{
if (sscanf (token, "%lu", &fieldno) != 1)
{
@@ -161,7 +163,7 @@ rust_get_disr_info (struct type *type, const
gdb_byte *valaddr,
member_type = TYPE_FIELD_TYPE (member_type, fieldno);
}
- if (token >= name + strlen (TYPE_FIELD_NAME (type, 0)))
+ if (token == NULL)
error (_("Invalid form for %s"), RUST_ENUM_PREFIX);
value = unpack_long (member_type, valaddr + embedded_offset);
--
2.8.3
-Manish
On Wed, Jun 29, 2016 at 4:01 PM, Manish Goregaokar <manish@mozilla.com> wrote:
> Fixed.
>
> From 6b577bb78279a453c77b08c5a1090d8dec1eeee7 Mon Sep 17 00:00:00 2001
> From: Manish Goregaokar <manish@mozilla.com>
> Date: Wed, 29 Jun 2016 15:42:28 +0530
> Subject: [PATCH] Use strtok_r instead of strsep in rust_get_disr_info
>
> strsep doesn't exist on Windows.
>
> 2016-06-29 Manish Goregaokar <manish@mozilla.com>
>
> gdb/ChangeLog:
> * rust-lang.c (rust_get_disr_info): Use strtok_r instead of strsep.
> ---
> gdb/rust-lang.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/gdb/rust-lang.c b/gdb/rust-lang.c
> index 23ddd5a..5910d4b 100644
> --- a/gdb/rust-lang.c
> +++ b/gdb/rust-lang.c
> @@ -121,7 +121,7 @@ rust_get_disr_info (struct type *type, const
> gdb_byte *valaddr,
> if (strncmp (TYPE_FIELD_NAME (type, 0), RUST_ENUM_PREFIX,
> strlen (RUST_ENUM_PREFIX)) == 0)
> {
> - char *name, *tail, *token;
> + char *tail, *token, *name, *saveptr;
> unsigned long fieldno;
> struct type *member_type;
> LONGEST value;
> @@ -145,7 +145,9 @@ rust_get_disr_info (struct type *type, const
> gdb_byte *valaddr,
> traversed in order to find the field (which may be several
> fields deep)
> and the variantname is the name of the variant of the case when the
> field is zero. */
> - while ((token = strsep (&tail, "$")) != NULL)
> + for (token = strtok_r (tail, "$", &saveptr);
> + token != NULL;
> + token = strtok_r (NULL, "$", &saveptr))
> {
> if (sscanf (token, "%lu", &fieldno) != 1)
> {
> @@ -161,8 +163,6 @@ rust_get_disr_info (struct type *type, const
> gdb_byte *valaddr,
> member_type = TYPE_FIELD_TYPE (member_type, fieldno);
> }
>
> - if (token >= name + strlen (TYPE_FIELD_NAME (type, 0)))
> - error (_("Invalid form for %s"), RUST_ENUM_PREFIX);
> value = unpack_long (member_type, valaddr + embedded_offset);
>
> if (value == 0)
> --
> 2.8.3
>
> -Manish
>
>
> On Wed, Jun 29, 2016 at 3:56 PM, Pedro Alves <palves@redhat.com> wrote:
>> (stepping in since this fixes a build breakage.)
>>
>> On 06/29/2016 11:13 AM, Manish Goregaokar wrote:
>>> strsep doesn't exist on windows
>>
>> "Windows", uppercase.
>>
>>>
>>> 2016-06-29 Manish Goregaokar <manish@mozilla.com>
>>>
>>> gdb/ChangeLog:
>>> * rust-lang.c (rust_get_disr_info): Use strtok_r instead of strsep
>>
>> Period at end of sentences. Likewise on the commit log,
>> while at it.
>>
>>> ---
>>> gdb/rust-lang.c | 10 +++++-----
>>> 1 file changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/gdb/rust-lang.c b/gdb/rust-lang.c
>>> index 23ddd5a..961fdca 100644
>>> --- a/gdb/rust-lang.c
>>> +++ b/gdb/rust-lang.c
>>> @@ -121,7 +121,7 @@ rust_get_disr_info (struct type *type, const
>>> gdb_byte *valaddr,
>>> if (strncmp (TYPE_FIELD_NAME (type, 0), RUST_ENUM_PREFIX,
>>> strlen (RUST_ENUM_PREFIX)) == 0)
>>> {
>>> - char *name, *tail, *token;
>>> + char *tail, *token, *name, *last = NULL;
>>
>> (I don't think you need to initialize this. If you get
>> a warning about it, it's likely revealing a bug.)
>>
>>> unsigned long fieldno;
>>> struct type *member_type;
>>> LONGEST value;
>>> @@ -138,6 +138,8 @@ rust_get_disr_info (struct type *type, const
>>> gdb_byte *valaddr,
>>> cleanup = make_cleanup (xfree, name);
>>> tail = name + strlen (RUST_ENUM_PREFIX);
>>>
>>> + token = strtok_r (tail, "$", &last);
>>> +
>>> /* The location of the value that doubles as a discriminant is
>>> stored in the name of the field, as
>>> RUST$ENCODED$ENUM$<fieldno>$<fieldno>$...$<variantname>
>>> @@ -145,7 +147,7 @@ rust_get_disr_info (struct type *type, const
>>> gdb_byte *valaddr,
>>> traversed in order to find the field (which may be several
>>> fields deep)
>>> and the variantname is the name of the variant of the case when the
>>> field is zero. */
>>> - while ((token = strsep (&tail, "$")) != NULL)
>>> + do
>>> {
>>> if (sscanf (token, "%lu", &fieldno) != 1)
>>
>> We'll now reach here even if token is NULL the first time.
>>
>>> {
>>> @@ -159,10 +161,8 @@ rust_get_disr_info (struct type *type, const
>>> gdb_byte *valaddr,
>>>
>>> embedded_offset += TYPE_FIELD_BITPOS (member_type, fieldno) / 8;
>>> member_type = TYPE_FIELD_TYPE (member_type, fieldno);
>>> - }
>>> + } while ((token = strtok_r (NULL, "$", &last)) != NULL);
>>
>> IMO, this calls for a "for" loop instead of a do/while. Like:
>>
>> - while ((token = strsep (&tail, "$")) != NULL)
>> + for (token = strtok_r (tail, "$", &saveptr);
>> + token != NULL;
>> + token = strtok_r (NULL, "$", &saveptr))
>> {
>>
>> Thanks,
>> Pedro Alves
>>