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

[Pedro Alves <palves@redhat.com>]

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