diff mbox

[RFA_v3,1/8] Add helper functions parse_flags and parse_flags_qcs

Message ID 20180624183708.888-2-philippe.waroquiers@skynet.be
State New
Headers show

Commit Message

Philippe Waroquiers June 24, 2018, 6:37 p.m. UTC
Add helper functions parse_flags and parse_flags_qcs.
parse_flags helper function allows to look for a set of flags at
the start of a string.
A flag must be given individually.

parse_flags_qcs is a specialised helper function to handle
the flags -q, -c and -s, that are used in the new command 'frame apply'
and in the command 'thread apply.

Modify number_or_range_parser::get_number to differentiate a
- followed by digits from a - followed by an alpha (i.e. a flag or an option).
That is needed for the addition of the [FLAG]... arguments to
thread apply ID... [FLAG]... COMMAND

Remove bool number_or_range_parser::m_finished, rather
implement the 'finished' logic inside number_or_range_parser::finished.
The new logic properly detects the end of parsing even if not at
end of the string. This ensures that number_or_range_parser::cur_tok
really points past the last parsed token when parsing is finished.
Before, it was always pointing at the end of the string.
As parsing now is finished directly when not positioned on a number,
number_is_in_list must do an error check before the loop getting all
numbers.

The error message for 'thread apply -$unknownconvvar p 1'
is now the more clear:
  Convenience variable must have integer value.
  Invalid thread ID: -$unknownconvvar p 1
instead of previously:
  negative value

gdb/ChangeLog
2018-06-24  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

	* cli-utils.c (number_or_range_parser::get_number): Only handle
	numbers or convenience var as numbers.
	(parse_flags): New function.
	(parse_flags_qcs): New function.
        (number_or_range_parser::finished): ensure parsing end is detected
        before end of string.
	* cli-utils.h (parse_flags): New function.
	(parse_flags_qcs): New function.
        (number_or_range_parser): Remove m_finished bool.
        (number_or_range_parser::skip_range): set m_in_range to false.
---
 gdb/cli/cli-utils.c | 98 +++++++++++++++++++++++++++++++++++++++++++--
 gdb/cli/cli-utils.h | 35 +++++++++++++---
 2 files changed, 124 insertions(+), 9 deletions(-)

Comments

Pedro Alves July 9, 2018, 7:14 p.m. UTC | #1
Hi Philippe,

This looks mostly good to me.  A few minor comments below,
and a suggestion.

On 06/24/2018 07:37 PM, Philippe Waroquiers wrote:

> 
> The error message for 'thread apply -$unknownconvvar p 1'
> is now the more clear:
>   Convenience variable must have integer value.
>   Invalid thread ID: -$unknownconvvar p 1
> instead of previously:
>   negative value
> 
> gdb/ChangeLog
> 2018-06-24  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
> 
> 	* cli-utils.c (number_or_range_parser::get_number): Only handle
> 	numbers or convenience var as numbers.
> 	(parse_flags): New function.
> 	(parse_flags_qcs): New function.
>         (number_or_range_parser::finished): ensure parsing end is detected
>         before end of string.
> 	* cli-utils.h (parse_flags): New function.
> 	(parse_flags_qcs): New function.
>         (number_or_range_parser): Remove m_finished bool.
>         (number_or_range_parser::skip_range): set m_in_range to false.

Note some tab vs spaces mixup above.

> ---
>  gdb/cli/cli-utils.c | 98 +++++++++++++++++++++++++++++++++++++++++++--
>  gdb/cli/cli-utils.h | 35 +++++++++++++---
>  2 files changed, 124 insertions(+), 9 deletions(-)
> 
> diff --git a/gdb/cli/cli-utils.c b/gdb/cli/cli-utils.c
> index c55b5035e4..6fef419216 100644
> --- a/gdb/cli/cli-utils.c
> +++ b/gdb/cli/cli-utils.c
> @@ -137,7 +137,6 @@ number_or_range_parser::number_or_range_parser (const char *string)
>  void
>  number_or_range_parser::init (const char *string)
>  {
> -  m_finished = false;
>    m_cur_tok = string;
>    m_last_retval = 0;
>    m_end_value = 0;
> @@ -169,7 +168,10 @@ number_or_range_parser::get_number ()
>        /* 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_cur_tok, '-');
> -      if (*m_cur_tok == '-')
> +      /* If get_number_trailer has found a -, it might be the start
> +	 of a command option.  So, do not parse a range if the - is
> +	 followed by an alpha.  */
> +      if (*m_cur_tok == '-' && !isalpha (*(m_cur_tok + 1)))
>  	{
>  	  const char **temp;
>  
> @@ -196,8 +198,17 @@ number_or_range_parser::get_number ()
>  	}
>      }
>    else
> -    error (_("negative value"));
> -  m_finished = *m_cur_tok == '\0';
> +    {
> +      if (isdigit (*(m_cur_tok + 1)))
> +	error (_("negative value"));
> +      if (*(m_cur_tok + 1) == '$')
> +	{
> +	  /* Convenience variable.  */
> +	  m_last_retval = ::get_number (&m_cur_tok);
> +	  if (m_last_retval < 0)
> +	    error (_("negative value"));
> +	}
> +    }
>    return m_last_retval;
>  }
>  
> @@ -215,6 +226,22 @@ number_or_range_parser::setup_range (int start_value, int end_value,
>    m_end_value = end_value;
>  }
>  
> +/* See documentation in cli-utils.h.  */
> +
> +bool
> +number_or_range_parser::finished () const
> +{
> +  /* Parsing is finished when at end of string or null string,
> +     or
> +     we are not in a range and not in front of an integer,
> +     negative integer, convenience var or negative convenience var.  */

Seems strange that "or" is in its own line.  Hit meta-q in emacs?

> +  return (m_cur_tok == NULL || *m_cur_tok == '\0')
> +    || (!m_in_range
> +	&& !(isdigit (*m_cur_tok) || *m_cur_tok == '$')
> +	&& !(*m_cur_tok == '-'
> +	     && (isdigit (*(m_cur_tok + 1)) || *(m_cur_tok + 1) == '$')));
> +}
> +

GNU's convention is to wrap the whole multi-line expression
in parens so that emacs indents the || in the second line under
"(m_curr_tok".  We don't really need the parens in the first
line, so that gives us:

bool
number_or_range_parser::finished () const
{
  /* Parsing is finished when at end of string or null string, or we
     are not in a range and not in front of an integer, negative
     integer, convenience var or negative convenience var.  */
  return (m_cur_tok == NULL || *m_cur_tok == '\0'
	  || (!m_in_range
	      && !(isdigit (*m_cur_tok) || *m_cur_tok == '$')
	      && !(*m_cur_tok == '-'
		   && (isdigit (*(m_cur_tok + 1)) || *(m_cur_tok + 1) == '$')));
}


You could also use m_cur_tok[1] instead of "*(m_cur_tok + 1)" if you
like to make it a little bit shorter.


>  /* Accept a number and a string-form list of numbers such as is 
>     accepted by get_number_or_range.  Return TRUE if the number is
>     in the list.
> @@ -230,6 +257,9 @@ number_is_in_list (const char *list, int number)
>      return 1;
>  
>    number_or_range_parser parser (list);
> +
> +  if (parser.finished ())
> +    error (_("Args must be numbers or '$' variables."));

I think it's better to spell out "Arguments".

> +
> +int
> +parse_flags_qcs (const char *which_command, const char **str,
> +		 bool *quiet, bool *cont, bool *silent)
> +{
> +  const char *flags = "qcs";
> +  int res;

"res" is not used.

> +
> +  switch (parse_flags (str, flags))
> +    {
> +    case 0:
> +      return 0;
> +    case 1:
> +      *quiet = true;
> +      break;
> +    case 2:
> +      *cont = true;
> +      break;
> +    case 3:
> +      *silent = true;
> +      break;
> +    default:
> +      gdb_assert (0);

Use gdb_assert_not_reached.

> +    }
> +
> +  if (*cont && *silent)
> +    error (_("%s: -c and -s are mutually exclusive"), which_command);
> +
> +  return 1;
> +}

> @@ -173,4 +170,32 @@ check_for_argument (char **str, const char *arg, int arg_len)
>  			     arg, arg_len);
>  }
>  
> +/* A helper function that looks for a set of flags at the start of a
> +   string.  The possible flags are given as a null terminated string.
> +   A flag in STR must either be at the end of the string,
> +   or be followed by whitespace.
> +   Returns 0 if no valid flag is found at the start of STR.
> +   Otherwise updates *STR, and returns N (which is > 0),
> +   such that FLAGS [N - 1] is the valid found flag.  */
> +extern int parse_flags (const char **str, const char *flags);
> +
> +/* A helper function that uses parse_flags to handle the flags qcs :
> +     A flag -q sets *QUIET to true.
> +     A flag -c sets *CONT to true.
> +     A flag -s sets *SILENT to true.
> +
> +   The caller is responsible to initialize *QUIET, *CONT, *SILENT to
> +   false before the (first) call to parse_flags_qcs.
> +   parse_flags_qcs can then be called iteratively to search for more
> +   valid flags, as part of a 'main parsing loop' searching for -q/-c/-s
> +   flags together with other flags and options.
> +
> +   Returns 1 and updates *STR and one of *QUIET, *CONT, *SILENT
> +   if it finds a valid flag.
> +   Returns 0 if no valid flag is found at the beginning of STR.

This can be bool and true/false, right?

> +
> +   Throws an error if a flag is found such that both *CONT and *SILENT
> +   are true.  */
> +extern int parse_flags_qcs (const char *which_command, const char **str,
> +			    bool *quiet, bool *cont, bool *silent);

I suspect that using a single structure instead of multiple output bools,
like this:

struct qcs_flags
{
  bool quiet = false;
  bool cont = false;
  bool silent = false;
};

extern bool parse_flags_qcs (const char *which_command, const char **str,
			    qcs_flags *flags);

would make the code a little clearer.  Note that that way the "= false"
default initializers are part of the type, no need to do that explicitly
at the callers.

Thanks,
Pedro Alves
diff mbox

Patch

diff --git a/gdb/cli/cli-utils.c b/gdb/cli/cli-utils.c
index c55b5035e4..6fef419216 100644
--- a/gdb/cli/cli-utils.c
+++ b/gdb/cli/cli-utils.c
@@ -137,7 +137,6 @@  number_or_range_parser::number_or_range_parser (const char *string)
 void
 number_or_range_parser::init (const char *string)
 {
-  m_finished = false;
   m_cur_tok = string;
   m_last_retval = 0;
   m_end_value = 0;
@@ -169,7 +168,10 @@  number_or_range_parser::get_number ()
       /* 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_cur_tok, '-');
-      if (*m_cur_tok == '-')
+      /* If get_number_trailer has found a -, it might be the start
+	 of a command option.  So, do not parse a range if the - is
+	 followed by an alpha.  */
+      if (*m_cur_tok == '-' && !isalpha (*(m_cur_tok + 1)))
 	{
 	  const char **temp;
 
@@ -196,8 +198,17 @@  number_or_range_parser::get_number ()
 	}
     }
   else
-    error (_("negative value"));
-  m_finished = *m_cur_tok == '\0';
+    {
+      if (isdigit (*(m_cur_tok + 1)))
+	error (_("negative value"));
+      if (*(m_cur_tok + 1) == '$')
+	{
+	  /* Convenience variable.  */
+	  m_last_retval = ::get_number (&m_cur_tok);
+	  if (m_last_retval < 0)
+	    error (_("negative value"));
+	}
+    }
   return m_last_retval;
 }
 
@@ -215,6 +226,22 @@  number_or_range_parser::setup_range (int start_value, int end_value,
   m_end_value = end_value;
 }
 
+/* See documentation in cli-utils.h.  */
+
+bool
+number_or_range_parser::finished () const
+{
+  /* Parsing is finished when at end of string or null string,
+     or
+     we are not in a range and not in front of an integer,
+     negative integer, convenience var or negative convenience var.  */
+  return (m_cur_tok == NULL || *m_cur_tok == '\0')
+    || (!m_in_range
+	&& !(isdigit (*m_cur_tok) || *m_cur_tok == '$')
+	&& !(*m_cur_tok == '-'
+	     && (isdigit (*(m_cur_tok + 1)) || *(m_cur_tok + 1) == '$')));
+}
+
 /* Accept a number and a string-form list of numbers such as is 
    accepted by get_number_or_range.  Return TRUE if the number is
    in the list.
@@ -230,6 +257,9 @@  number_is_in_list (const char *list, int number)
     return 1;
 
   number_or_range_parser parser (list);
+
+  if (parser.finished ())
+    error (_("Args must be numbers or '$' variables."));
   while (!parser.finished ())
     {
       int gotnum = parser.get_number ();
@@ -304,3 +334,63 @@  check_for_argument (const char **str, const char *arg, int arg_len)
     }
   return 0;
 }
+
+/* See documentation in cli-utils.h.  */
+
+int
+parse_flags (const char **str, const char *flags)
+{
+  const char *p = skip_spaces (*str);
+
+  if (p[0] == '-'
+      && isalpha (p[1])
+      && (p[2] == '\0' || isspace (p[2])))
+    {
+      const char pf = p[1];
+      const char *f = flags;
+
+      while (*f != '\0')
+	{
+	  if (*f == pf)
+	    {
+	      *str = skip_spaces (p + 2);
+	      return f - flags + 1;
+	    }
+	  f++;
+	}
+    }
+
+  return 0;
+}
+
+/* See documentation in cli-utils.h.  */
+
+int
+parse_flags_qcs (const char *which_command, const char **str,
+		 bool *quiet, bool *cont, bool *silent)
+{
+  const char *flags = "qcs";
+  int res;
+
+  switch (parse_flags (str, flags))
+    {
+    case 0:
+      return 0;
+    case 1:
+      *quiet = true;
+      break;
+    case 2:
+      *cont = true;
+      break;
+    case 3:
+      *silent = true;
+      break;
+    default:
+      gdb_assert (0);
+    }
+
+  if (*cont && *silent)
+    error (_("%s: -c and -s are mutually exclusive"), which_command);
+
+  return 1;
+}
diff --git a/gdb/cli/cli-utils.h b/gdb/cli/cli-utils.h
index e34ee0df37..56be58e955 100644
--- a/gdb/cli/cli-utils.h
+++ b/gdb/cli/cli-utils.h
@@ -75,8 +75,7 @@  public:
 		    const char *end_ptr);
 
   /* Returns true if parsing has completed.  */
-  bool finished () const
-  { return m_finished; }
+  bool finished () const;
 
   /* Return the string being parsed.  When parsing has finished, this
      points past the last parsed token.  */
@@ -96,6 +95,7 @@  public:
   {
     gdb_assert (m_in_range);
     m_cur_tok = m_end_ptr;
+    m_in_range = false;
   }
 
 private:
@@ -103,9 +103,6 @@  private:
   number_or_range_parser (const number_or_range_parser &);
   number_or_range_parser &operator= (const number_or_range_parser &);
 
-  /* True if parsing has completed.  */
-  bool m_finished;
-
   /* The string being parsed.  When parsing has finished, this points
      past the last parsed token.  */
   const char *m_cur_tok;
@@ -173,4 +170,32 @@  check_for_argument (char **str, const char *arg, int arg_len)
 			     arg, arg_len);
 }
 
+/* A helper function that looks for a set of flags at the start of a
+   string.  The possible flags are given as a null terminated string.
+   A flag in STR must either be at the end of the string,
+   or be followed by whitespace.
+   Returns 0 if no valid flag is found at the start of STR.
+   Otherwise updates *STR, and returns N (which is > 0),
+   such that FLAGS [N - 1] is the valid found flag.  */
+extern int parse_flags (const char **str, const char *flags);
+
+/* A helper function that uses parse_flags to handle the flags qcs :
+     A flag -q sets *QUIET to true.
+     A flag -c sets *CONT to true.
+     A flag -s sets *SILENT to true.
+
+   The caller is responsible to initialize *QUIET, *CONT, *SILENT to
+   false before the (first) call to parse_flags_qcs.
+   parse_flags_qcs can then be called iteratively to search for more
+   valid flags, as part of a 'main parsing loop' searching for -q/-c/-s
+   flags together with other flags and options.
+
+   Returns 1 and updates *STR and one of *QUIET, *CONT, *SILENT
+   if it finds a valid flag.
+   Returns 0 if no valid flag is found at the beginning of STR.
+
+   Throws an error if a flag is found such that both *CONT and *SILENT
+   are true.  */
+extern int parse_flags_qcs (const char *which_command, const char **str,
+			    bool *quiet, bool *cont, bool *silent);
 #endif /* CLI_UTILS_H */