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

[Pedro Alves <palves at redhat dot com>]

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