Re: [RFA 19/22] Convert tid_range_parser to class

Pedro Alves
2016-10-04T19:24:43Z

On 10/01/2016 12:23 AM, Pedro Alves wrote:
> On 09/30/2016 03:07 PM, Tom Tromey wrote:
>>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
>>
>> Pedro> Whoops, we ended up duplicating work here.
>>
>> Oops :) But no big deal.
>>
>> Pedro> Additional differences compared to yours are:
>> Pedro> - "bool" instead of "int".
>> Pedro> - "m_" prefixes.
>>
>> Aha, the m_ prefix. Cancel that earlier question...
>>
>> Pedro> Let me know what you think. It's super fine with me to rebase
>> Pedro> mine on top of yours. Did you have a follow up patch for
>> Pedro> get_number_or_range too, perhaps?
>>
>> I don't. How about I just drop my patch? That seems simplest to me.
>
> OK. I'll look at your patch in more detail and see what could be
> merged in. I'll add your name to the ChangeLog.
>
>>
>> Pedro> void
>> Pedro> -init_number_or_range (struct get_number_or_range_state *state,
>> Pedro> - const char *string)
>> Pedro> +number_or_range_parser::init (const char *string)
>> Pedro> {
>> Pedro> - memset (state, 0, sizeof (*state));
>> Pedro> - state->string = string;
>> Pedro> + memset (this, 0, sizeof (*this));
>>
>> I think it's better to do explicit initialization.
>
> I'll do this.
>
>> This will bite if this class ever is changed to have a vtable (unlikely
>> but it's better, IMO, to set a good example).
>
> Yeah, we'd notice bad things immediately, but I agree with setting
> a good example.

I did the change below locally. I was looking at string() vs
get_string(), and not being super happy with either. string()
is maybe too generic, doesn't really convey what the string is about,
and the get_ in get_string() makes me go "the other get_ members are about
parsing/producing a number, while this is peeking at parser state". So in
the style of xkcd 927, I thought of an alternative name -- cur_tok(),
which seems to be at least somewhat of a common field/method name in
parser lingo, according to google.

I've also renamed get_next to get_number:

- int gotnum = parser.get_next ();
+ int gotnum = parser.get_number ();

which seems more consistent.

Got rid of the memset too.

I'm now looking at finished() vs is_qualified() and pondering
whether to do something about that little inconsistency. :-)

Anyway, I have to disappear now. Will continue later.

From 4ae2903eee5b5c04f56658a02c486ba4c8edcd5f Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Tue, 4 Oct 2016 20:14:56 +0100
Subject: [PATCH] address review

---
gdb/breakpoint.c | 6 +++---
gdb/cli/cli-utils.c | 28 ++++++++++++++++------------
gdb/cli/cli-utils.h | 18 +++++++++---------
gdb/inferior.c | 6 +++---
gdb/linespec.c | 2 +-
gdb/memattr.c | 6 +++---
gdb/printcmd.c | 4 ++--
gdb/reverse.c | 4 ++--
gdb/thread.c | 7 +++----
gdb/tid-parse.c | 32 ++++++++++++++++----------------
gdb/tid-parse.h | 12 ++++++------
11 files changed, 64 insertions(+), 61 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 6b47541..32d6a95 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -14783,10 +14783,10 @@ map_breakpoint_numbers (char *args, void (*function) (struct breakpoint *,

while (!parser.finished ())
{
- const char *p = parser.string ();
+ const char *p = parser.cur_tok ();
bool match = false;

- num = parser.get_next ();
+ num = parser.get_number ();
if (num == 0)
{
warning (_("bad breakpoint number at or near '%s'"), p);
@@ -15639,7 +15639,7 @@ get_tracepoint_by_number (char **arg,
if (parser != NULL)
{
gdb_assert (!parser->finished ());
- tpnum = parser->get_next ();
+ tpnum = parser->get_number ();
}
else if (arg == NULL || *arg == NULL || ! **arg)
tpnum = tracepoint_count;
diff --git a/gdb/cli/cli-utils.c b/gdb/cli/cli-utils.c
index 379f341..0fb68f2 100644
--- a/gdb/cli/cli-utils.c
+++ b/gdb/cli/cli-utils.c
@@ -131,14 +131,18 @@ number_or_range_parser::number_or_range_parser (const char *string)
void
number_or_range_parser::init (const char *string)
{
- memset (this, 0, sizeof (*this));
- m_string = string;
+ m_finished = false;
+ m_cur_tok = string;
+ m_last_retval = 0;
+ m_end_value = 0;
+ m_end_ptr = NULL;
+ m_in_range = false;
}

/* See documentation in cli-utils.h. */

int
-number_or_range_parser::get_next ()
+number_or_range_parser::get_number ()
{
if (m_in_range)
{
@@ -150,16 +154,16 @@ number_or_range_parser::get_next ()
if (++m_last_retval == m_end_value)
{
/* End of range reached; advance token pointer. */
- m_string = m_end_ptr;
+ m_cur_tok = m_end_ptr;
m_in_range = false;
}
}
- else if (*m_string != '-')
+ else if (*m_cur_tok != '-')
{
- /* Default case: state->string is pointing either to a solo
+ /* Default case: state->m_cur_tok is pointing either to a solo
number, or to the first number of a range. */
- m_last_retval = get_number_trailer (&m_string, '-');
- if (*m_string == '-')
+ m_last_retval = get_number_trailer (&m_cur_tok, '-');
+ if (*m_cur_tok == '-')
{
const char **temp;

@@ -168,7 +172,7 @@ number_or_range_parser::get_next ()
and also remember the end of the final token. */

temp = &m_end_ptr;
- m_end_ptr = skip_spaces_const (m_string + 1);
+ m_end_ptr = skip_spaces_const (m_cur_tok + 1);
m_end_value = get_number_const (temp);
if (m_end_value < m_last_retval)
{
@@ -179,7 +183,7 @@ number_or_range_parser::get_next ()
/* Degenerate range (number1 == number2). Advance the
token pointer so that the range will be treated as a
single number. */
- m_string = m_end_ptr;
+ m_cur_tok = m_end_ptr;
}
else
m_in_range = true;
@@ -187,7 +191,7 @@ number_or_range_parser::get_next ()
}
else
error (_("negative value"));
- m_finished = *m_string == '\0';
+ m_finished = *m_cur_tok == '\0';
return m_last_retval;
}

@@ -222,7 +226,7 @@ number_is_in_list (const char *list, int number)
number_or_range_parser parser (list);
while (!parser.finished ())
{
- int gotnum = parser.get_next ();
+ int gotnum = parser.get_number ();

if (gotnum == 0)
error (_("Args must be numbers or '$' variables."));
diff --git a/gdb/cli/cli-utils.h b/gdb/cli/cli-utils.h
index 41c3a58..ab3f122 100644
--- a/gdb/cli/cli-utils.h
+++ b/gdb/cli/cli-utils.h
@@ -62,11 +62,11 @@ public:
each call it will return the next value in the range.

At the beginning of parsing a range, the char pointer
- STATE->string will be advanced past <number1> and left pointing
- at the '-' token. Subsequent calls will not advance the pointer
- until the range is completed. The call that completes the range
- will advance the pointer past <number2>. */
- int get_next ();
+ STATE->m_cur_tok will be advanced past <number1> and left
+ pointing at the '-' token. Subsequent calls will not advance the
+ pointer until the range is completed. The call that completes
+ the range will advance the pointer past <number2>. */
+ int get_number ();

/* Setup internal state such that get_next() returns numbers in the
START_VALUE to END_VALUE range. END_PTR is where the string is
@@ -80,8 +80,8 @@ public:

/* Return the string being parsed. When parsing has finished, this
points past the last parsed token. */
- const char *string ()
- { return m_string; }
+ const char *cur_tok ()
+ { return m_cur_tok; }

/* True when parsing a range. */
bool in_range ()
@@ -95,7 +95,7 @@ public:
void skip_range ()
{
gdb_assert (m_in_range);
- m_string = m_end_ptr;
+ m_cur_tok = m_end_ptr;
}

private:
@@ -104,7 +104,7 @@ private:

/* The string being parsed. When parsing has finished, this points
past the last parsed token. */
- const char *m_string;
+ const char *m_cur_tok;

/* Last value returned. */
int m_last_retval;
diff --git a/gdb/inferior.c b/gdb/inferior.c
index 836adc9..1602483 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -659,7 +659,7 @@ detach_inferior_command (char *args, int from_tty)
number_or_range_parser parser (args);
while (!parser.finished ())
{
- int num = parser.get_next ();
+ int num = parser.get_number ();

if (!valid_gdb_inferior_id (num))
{
@@ -698,7 +698,7 @@ kill_inferior_command (char *args, int from_tty)
number_or_range_parser parser (args);
while (!parser.finished ())
{
- int num = parser.get_next ();
+ int num = parser.get_number ();

if (!valid_gdb_inferior_id (num))
{
@@ -790,7 +790,7 @@ remove_inferior_command (char *args, int from_tty)
number_or_range_parser parser (args);
while (!parser.finished ())
{
- int num = parser.get_next ();
+ int num = parser.get_number ();
struct inferior *inf = find_inferior_id (num);

if (inf == NULL)
diff --git a/gdb/linespec.c b/gdb/linespec.c
index 7b1a77e..5182b45 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -1411,7 +1411,7 @@ decode_line_2 (struct linespec_state *self,
number_or_range_parser parser (args);
while (!parser.finished ())
{
- int num = parser.get_next ();
+ int num = parser.get_number ();

if (num == 0)
error (_("canceled"));
diff --git a/gdb/memattr.c b/gdb/memattr.c
index a33993a..1c5c48f 100644
--- a/gdb/memattr.c
+++ b/gdb/memattr.c
@@ -581,7 +581,7 @@ mem_enable_command (char *args, int from_tty)
number_or_range_parser parser (args);
while (!parser.finished ())
{
- num = parser.get_next ();
+ num = parser.get_number ();
mem_enable (num);
}
}
@@ -625,7 +625,7 @@ mem_disable_command (char *args, int from_tty)
number_or_range_parser parser (args);
while (!parser.finished ())
{
- int num = parser.get_next ();
+ int num = parser.get_number ();
mem_disable (num);
}
}
@@ -676,7 +676,7 @@ mem_delete_command (char *args, int from_tty)
number_or_range_parser parser (args);
while (!parser.finished ())
{
- int num = parser.get_next ();
+ int num = parser.get_number ();
mem_delete (num);
}

diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index 15f2395..c7f477b 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -1885,9 +1885,9 @@ map_display_numbers (char *args,

while (!parser.finished ())
{
- const char *p = parser.string ();
+ const char *p = parser.cur_tok ();

- num = parser.get_next ();
+ num = parser.get_number ();
if (num == 0)
warning (_("bad display number at or near '%s'"), p);
else
diff --git a/gdb/reverse.c b/gdb/reverse.c
index 461d1a4..2c1abdc 100644
--- a/gdb/reverse.c
+++ b/gdb/reverse.c
@@ -233,7 +233,7 @@ delete_bookmark_command (char *args, int from_tty)
number_or_range_parser parser (args);
while (!parser.finished ())
{
- int num = parser.get_next ();
+ int num = parser.get_number ();
if (!delete_one_bookmark (num))
/* Not found. */
warning (_("No bookmark #%d."), num);
@@ -329,7 +329,7 @@ bookmarks_info (char *args, int from_tty)
number_or_range_parser parser (args);
while (!parser.finished ())
{
- int bnum = parser.get_next ();
+ int bnum = parser.get_number ();
bookmark_1 (bnum);
}
}
diff --git a/gdb/thread.c b/gdb/thread.c
index 819119d..f376211 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -1837,7 +1837,7 @@ thread_apply_command (char *tidlist, int from_tty)

if (!parser.get_tid_range (&inf_num, &thr_start, &thr_end))
{
- cmd = (char *) parser.string ();
+ cmd = (char *) parser.cur_tok ();
break;
}
}
@@ -1856,7 +1856,7 @@ thread_apply_command (char *tidlist, int from_tty)
make_cleanup_restore_current_thread ();

parser.init (tidlist, current_inferior ()->num);
- while (!parser.finished () && parser.string () < cmd)
+ while (!parser.finished () && parser.cur_tok () < cmd)
{
struct thread_info *tp = NULL;
struct inferior *inf;
@@ -1888,8 +1888,7 @@ thread_apply_command (char *tidlist, int from_tty)

if (tp == NULL)
{
- if (show_inferior_qualified_tids ()
- || parser.qualified ())
+ if (show_inferior_qualified_tids () || parser.is_qualified ())
warning (_("Unknown thread %d.%d"), inf_num, thr_num);
else
warning (_("Unknown thread %d"), thr_num);
diff --git a/gdb/tid-parse.c b/gdb/tid-parse.c
index 9e49eba..28c5286 100644
--- a/gdb/tid-parse.c
+++ b/gdb/tid-parse.c
@@ -125,7 +125,7 @@ void
tid_range_parser::init (const char *tidlist, int default_inferior)
{
m_state = STATE_INFERIOR;
- m_string = tidlist;
+ m_cur_tok = tidlist;
m_inf_num = 0;
m_qualified = 0;
m_default_inferior = default_inferior;
@@ -139,7 +139,7 @@ tid_range_parser::finished ()
switch (m_state)
{
case STATE_INFERIOR:
- return *m_string == '\0';
+ return *m_cur_tok == '\0';
case STATE_THREAD_RANGE:
case STATE_STAR_RANGE:
return m_range_parser.finished ();
@@ -151,15 +151,15 @@ tid_range_parser::finished ()
/* See tid-parse.h. */

const char *
-tid_range_parser::string ()
+tid_range_parser::cur_tok ()
{
switch (m_state)
{
case STATE_INFERIOR:
- return m_string;
+ return m_cur_tok;
case STATE_THREAD_RANGE:
case STATE_STAR_RANGE:
- return m_range_parser.string ();
+ return m_range_parser.cur_tok ();
}

gdb_assert_not_reached (_("unhandled state"));
@@ -172,13 +172,13 @@ tid_range_parser::skip_range ()
|| m_state == STATE_STAR_RANGE);

m_range_parser.skip_range ();
- init (m_range_parser.string (), m_default_inferior);
+ init (m_range_parser.cur_tok (), m_default_inferior);
}

/* See tid-parse.h. */

bool
-tid_range_parser::qualified ()
+tid_range_parser::is_qualified ()
{
return m_qualified;
}
@@ -196,9 +196,9 @@ tid_range_parser::get_tid_or_range (int *inf_num,
const char *p;
const char *space;

- space = skip_to_space (m_string);
+ space = skip_to_space (m_cur_tok);

- p = m_string;
+ p = m_cur_tok;
while (p < space && *p != '.')
p++;
if (p < space)
@@ -206,8 +206,8 @@ tid_range_parser::get_tid_or_range (int *inf_num,
const char *dot = p;

/* Parse number to the left of the dot. */
- p = m_string;
- m_inf_num = get_positive_number_trailer (&p, '.', m_string);
+ p = m_cur_tok;
+ m_inf_num = get_positive_number_trailer (&p, '.', m_cur_tok);
if (m_inf_num == 0)
return 0;

@@ -221,7 +221,7 @@ tid_range_parser::get_tid_or_range (int *inf_num,
{
m_inf_num = m_default_inferior;
m_qualified = false;
- p = m_string;
+ p = m_cur_tok;
}

m_range_parser.init (p);
@@ -237,9 +237,9 @@ tid_range_parser::get_tid_or_range (int *inf_num,
}

*inf_num = m_inf_num;
- *thr_start = m_range_parser.get_next ();
+ *thr_start = m_range_parser.get_number ();
if (*thr_start < 0)
- error (_("negative value: %s"), m_string);
+ error (_("negative value: %s"), m_cur_tok);
if (*thr_start == 0)
{
m_state = STATE_INFERIOR;
@@ -252,7 +252,7 @@ tid_range_parser::get_tid_or_range (int *inf_num,
if (!m_range_parser.in_range ())
{
m_state = STATE_INFERIOR;
- m_string = m_range_parser.string ();
+ m_cur_tok = m_range_parser.cur_tok ();

if (thr_end != NULL)
*thr_end = *thr_start;
@@ -316,7 +316,7 @@ tid_is_in_list (const char *list, int default_inferior,
int tmp_inf, tmp_thr_start, tmp_thr_end;

if (!parser.get_tid_range (&tmp_inf, &tmp_thr_start, &tmp_thr_end))
- invalid_thread_id_error (parser.string ());
+ invalid_thread_id_error (parser.cur_tok ());
if (tmp_inf == inf_num
&& tmp_thr_start <= thr_num && thr_num <= tmp_thr_end)
return 1;
diff --git a/gdb/tid-parse.h b/gdb/tid-parse.h
index 764facc..4ff74b7 100644
--- a/gdb/tid-parse.h
+++ b/gdb/tid-parse.h
@@ -74,7 +74,7 @@ public:
exists).

At the beginning of parsing a thread range, the char pointer
- PARSER->string will be advanced past <thread_number1> and left
+ PARSER->m_cur_tok will be advanced past <thread_number1> and left
pointing at the '-' token. Subsequent calls will not advance the
pointer until the range is completed. The call that completes
the range will advance the pointer past <thread_number2>.
@@ -116,9 +116,9 @@ public:
/* Returns true if parsing has completed. */
bool finished ();

- /* Return the string being parsed. When parsing has finished, this
- points past the last parsed token. */
- const char *string ();
+ /* Return the current token being parsed. When parsing has
+ finished, this points past the last parsed token. */
+ const char *cur_tok ();

/* When parsing a range, advance past the final token in the
range. */
@@ -126,7 +126,7 @@ public:

/* True if the TID last parsed was explicitly inferior-qualified.
IOW, whether the spec specified an inferior number explicitly. */
- bool qualified ();
+ bool is_qualified ();

private:
bool get_tid_or_range (int *inf_num, int *thr_start, int *thr_end);
@@ -147,7 +147,7 @@ private:

/* The string being parsed. When parsing has finished, this points
past the last parsed token. */
- const char *m_string;
+ const char *m_cur_tok;

/* The range parser state when we're parsing the thread number
sub-component. */
--
2.5.5