[RFA_v2,1/8] Add helper functions check_for_flags and check_for_flags_vqcs

  Add helper functions check_for_flags and check_for_flags_vqcs
check_for_flags helper function allows to look for a set of flags at
the start of a string.
Each flag must be given individually.

check_for_flags_vqcs is a specialised helper function to handle
the flags vqcs, 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 a flag (i.e. an alpha).
That is needed for the addition of the optional FLAGS... arguments to
thread apply ID... [FLAGS...] COMMAND

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-05  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

	* cli-utils.c (number_or_range_parser::get_number): Only handle
	numbers or convenience var as numbers.
	(check_for_flags): New function.
	(check_for_flags_vqcs): New function.
	* cli-utils.h (check_for_flags): New function.
	(check_for_flags_vqcs): New function.
---
 gdb/cli/cli-utils.c | 122 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 gdb/cli/cli-utils.h |  30 +++++++++++++
 2 files changed, 150 insertions(+), 2 deletions(-)
  

Hi Philippe,

Been taking a better look at this, finally.

On 06/05/2018 09:48 PM, Philippe Waroquiers wrote:
> Add helper functions check_for_flags and check_for_flags_vqcs
> check_for_flags helper function allows to look for a set of flags at
> the start of a string.
> Each flag must be given individually.
> 
> check_for_flags_vqcs is a specialised helper function to handle
> the flags vqcs, 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 a flag (i.e. an alpha).
> That is needed for the addition of the optional FLAGS... arguments to
> thread apply ID... [FLAGS...] COMMAND
> 
> 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-05  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
> 
> 	* cli-utils.c (number_or_range_parser::get_number): Only handle
> 	numbers or convenience var as numbers.
> 	(check_for_flags): New function.
> 	(check_for_flags_vqcs): New function.
> 	* cli-utils.h (check_for_flags): New function.
> 	(check_for_flags_vqcs): New function.

I'm not super happy with this design, because first it is
still at places biased toward "flags" instead of "command
options", and most importantly because it doesn't seem to
make it easy to add further options to commands that
use check_for_flags_vqcs, without the artificial limitation
of requiring all vqcs flags specified together.  E.g., what if we want to
add an option like "-foo NUM" to "thread apply" for example.

I'd seem to me that at least an iterative function which
could be interleaved with other options would be better.
Something like:

struct vqcs_flags
{
  /* Number of times -c was seen.  */
  int c = 0;

  /* Number of times -v was seen.  */
  int v = 0;
};

int parse_flags_vqcs (const char **args, vqcs_flags *flags);

and then

  vqcs_flags flags;

  while (*args != '\0')
    {
       if (parse_flags_vqcs (&args, &flags))
         continue;

       /* check other options here.  error if unknown.  */
    }

maybe even interleave the number-or-range parsing
in that loop.

> ---
>  gdb/cli/cli-utils.c | 122 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>  gdb/cli/cli-utils.h |  30 +++++++++++++
>  2 files changed, 150 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/cli/cli-utils.c b/gdb/cli/cli-utils.c
> index c55b5035e4..7ce923a060 100644
> --- a/gdb/cli/cli-utils.c
> +++ b/gdb/cli/cli-utils.c
> @@ -169,7 +169,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 flags.  So, do not parse a range if the - is followed by
> +	 an alpha.  */

I'd prefer to say "start of a command option" instead of "start of
flags".  (comment applies throughout)

> +      if (*m_cur_tok == '-' && !isalpha (*(m_cur_tok + 1)))
>  	{
>  	  const char **temp;
>  
> @@ -196,7 +199,17 @@ number_or_range_parser::get_number ()
>  	}
>      }
>    else
> -    error (_("negative value"));
> +    {
> +      if (isdigit (*(m_cur_tok + 1)))
> +	error (_("negative value"));
> +      if (*(m_cur_tok + 1) == '$')
> +	{
> +	  // Convenience variable.

We use /**/ style throughout.

> +	  m_last_retval = ::get_number (&m_cur_tok);
> +	  if (m_last_retval < 0)
> +	    error (_("negative value"));
> +	}
> +    }
>    m_finished = *m_cur_tok == '\0';
>    return m_last_retval;
>  }
> @@ -304,3 +317,108 @@ check_for_argument (const char **str, const char *arg, int arg_len)
>      }
>    return 0;
>  }
> +
> +/* See documentation in cli-utils.h.  */
> +
> +int
> +check_for_flags (const char **str, const char *flags,
> +		 int *flags_counts)
> +{
> +  const char *p = skip_spaces (*str);
> +  bool flag_found = false;
> +  bool all_valid = true;
> +
> +  /* First set the flags_counts to 0.  */
> +  memset (flags_counts, 0, sizeof (flags_counts[0]) * strlen (flags));
> +
> +  while (*p == '-' && all_valid)
> +    {
> +      const char pf = *(p + 1);
> +
> +      /* If - is followed by anything else than an alpha (including \0),

I think "anything other than" is more idiomatic.

> +	 then we do not have a flag.  This also cover the case of a command
> +	 that accepts optional flags followed by a negative integer.
> +	 In such a case, we will not handle a negative integer as invalid
> +	 flags : rather let the caller handle the negative integer.  */
> +      if (!isalpha (pf))
> +	break;
> +
> +      /* Check that what follows pf is the end of string, or a space.  */

s/pf/PF/

> +      if (*(p + 2) != '\0' && !isspace (*(p + 2)))
> +	{
> +	  all_valid = false;
> +	  break;
> +	}
> +
> +      /* We have an alpha pf flag : validate the flag, and update

s/pf/PF/

> +	 flags_counts for a valid flag.  */
> +      const char *f = flags;
> +      bool valid = false;
> +
> +      while (*f != '\0')
> +	{
> +	  valid = *f == pf;
> +	  if (valid)
> +	    {
> +	      flags_counts[f - flags]++;
> +	      flag_found = true;
> +	      p += 2;
> +	      p = skip_spaces (p);
> +	      break;
> +	    }
> +	  f++;
> +	}
> +
> +      all_valid = all_valid && valid;
> +    }
> +
> +  if (all_valid)
> +    {
> +      if (flag_found)
> +	{
> +	  *str = p;
> +	  return 1;
> +	}
> +      else
> +	return 0;
> +    }
> +  else
> +    return -1;
> +}
> +
> +/* See documentation in cli-utils.h.  */
> +
> +int
> +check_for_flags_vqcs (const char *which_command, const char **str,
> +		      int *print_what_v, int max_v,
> +		      bool *cont, bool *silent)
> +{
> +  const char *flags = "vqcs";
> +  int flags_counts[strlen (flags)];

Variable-length arrays are not standard C++, but I'll ignore.  ;-)

> +  int res;
> +
> +  *cont = false;
> +  *silent = false;
> +
> +  res = check_for_flags (str, flags, flags_counts);
> +  if (res == 0)
> +    return 0;
> +  if (res == -1)
> +    error (_("%s only accepts flags %s given individually"),
> +	   which_command, flags);

I think this error message might look a bit odd.  What does
it really mean?

> +  gdb_assert (res == 1);
> +
> +  *print_what_v = *print_what_v + flags_counts[0] - flags_counts[1];
> +  if (*print_what_v < 0)
> +    *print_what_v = 0;
> +  if (*print_what_v > max_v)
> +    *print_what_v = max_v;
> +
> +  *cont = flags_counts[2] > 0;
> +  *silent = flags_counts[3] > 0;
> +  if (*cont && *silent)
> +    error (_("%s: flags c and s are mutually exclusive"),
> +	   which_command);

Here I think say "options -c and -s" or just "-c and -s"
instead of "flags c and s".

> +
> +  return res;
> +}
> diff --git a/gdb/cli/cli-utils.h b/gdb/cli/cli-utils.h
> index e34ee0df37..9e74725393 100644
> --- a/gdb/cli/cli-utils.h
> +++ b/gdb/cli/cli-utils.h
> @@ -173,4 +173,34 @@ 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 1 if it finds only valid flags, 0 if no flags are found,
> +   -1 if invalid flags are found.
> +   FLAGS_COUNTS must be an int array of length equal to strlen (FLAGS).
> +   Sets *FLAGS_COUNTS to the number of times the corresponding flag
> +   was found in *STR.
> +   If only valid flags are found, it updates *STR.  Note that a negative
> +   integer (e.g. -123) will not be considered as an invalid flag.  */
> +extern int check_for_flags (const char **str, const char *flags,
> +			    int *flags_counts);
> +
> +/* A helper function that uses check_for_flags to handle the flags vqcs :
> +     A flag -v increases *print_what_v.
> +     A flag -q decreases *print_what_v.
> +     A flag -c sets *cont to true, otherwise to false.
> +     A flag -s sets *silent to true, otherwise to false.
> +   The caller must initialise *PRINT_WHAT_V to the default verbosity.

We use US English, initialize.

> +   MAX_V gives the maximum verbosity : the value returned in *PRINT_WHAT_V
> +   will always be >= 0 and <= MAX_V.
> +   WHICH_COMMAND is used in the error message in case invalid flags are found.
> +
> +   Returns 1 and updates *STR if it finds only valid flags.
> +   Returns 0 if no flags are found.
> +   Raises an error if invalid flags are found.  */
> +extern int check_for_flags_vqcs (const char *which_command, const char **str,
> +				 int *print_what_v, int max_v,
> +				 bool *cont, bool *silent);
>  #endif /* CLI_UTILS_H */
> 

Thanks,
Pedro Alves
  

On Wed, 2018-06-13 at 20:52 +0100, Pedro Alves wrote:
> Hi Philippe,
> 
> Been taking a better look at this, finally.
Thanks for the review, I will handle all the comments,
I have some feedback/questions on a few of them below.


> > 	* cli-utils.c (number_or_range_parser::get_number): Only handle
> > 	numbers or convenience var as numbers.
> > 	(check_for_flags): New function.
> > 	(check_for_flags_vqcs): New function.
> > 	* cli-utils.h (check_for_flags): New function.
> > 	(check_for_flags_vqcs): New function.
> 
> I'm not super happy with this design, because first it is
> still at places biased toward "flags" instead of "command
> options", and most importantly because it doesn't seem to
> make it easy to add further options to commands that
> use check_for_flags_vqcs, without the artificial limitation
> of requiring all vqcs flags specified together.  E.g., what if we want to
> add an option like "-foo NUM" to "thread apply" for example.

Yes I agree, the current way to give the vqcs option is too unflexible,
so I will rework based on the iterative function you suggest below.

Just one clarification: I assume that by 'at places biased toward "flags"',
you mean that the names should be 'check_for_options_vqcs' ?
Otherwise, can you explain what you mean with bias ?

> 
> I'd seem to me that at least an iterative function which
> could be interleaved with other options would be better.
> Something like:
> 
> struct vqcs_flags
> {
>   /* Number of times -c was seen.  */
>   int c = 0;
> 
>   /* Number of times -v was seen.  */
>   int v = 0;
> };
> 
> int parse_flags_vqcs (const char **args, vqcs_flags *flags);
> 
> and then
> 
>   vqcs_flags flags;
> 
>   while (*args != '\0')
>     {
>        if (parse_flags_vqcs (&args, &flags))
>          continue;
> 
>        /* check other options here.  error if unknown.  */
>     }
> 
> maybe even interleave the number-or-range parsing
> in that loop.
Probably that can be done, but isn't this a little bit cumbersome ?
E.g. it means the help 
  thread apply ID... [OPTIONS] COMMAND
will become something like
  thread apply OPTIONS_OR_ID... COMMAND
and then we have to explain what OPTIONS_OR_ID can be.

(and I guess such syntax might make the 'generalised option parser'
more difficult to implement/use : we better keep ID... as a 
'positional argument' for an easy conversion to an generalised
option/arg parser).

So, I am more keen to keep
   thread apply ID... [OPTIONS] COMMAND
(note: we can always change it in a backward compatible
way in the future if we really believe mixing OPTIONS and ID...
has a strong value).

+  res = check_for_flags (str, flags, flags_counts);
> > +  if (res == 0)
> > +    return 0;
> > +  if (res == -1)
> > +    error (_("%s only accepts flags %s given individually"),
> > +	   which_command, flags);
> 
> I think this error message might look a bit odd.  What does
> it really mean?
It catches the below erroneous case, but probably this will become
'unknown option -vc' when I do the iterative design:
  (gdb) thread apply all -vc p 1
  thread apply all only accepts flags vqcs given individually
  (gdb) 

(I think that the generalised option/arg parser you have prototyped
will be a nice help to have a consistent and easier to code
gdb command parsing :).

Thanks

Philippe
  

On 06/14/2018 10:40 PM, Philippe Waroquiers wrote:
> On Wed, 2018-06-13 at 20:52 +0100, Pedro Alves wrote:
>> Hi Philippe,
>>
>> Been taking a better look at this, finally.
> Thanks for the review, I will handle all the comments,
> I have some feedback/questions on a few of them below.
> 
> 
>>> 	* cli-utils.c (number_or_range_parser::get_number): Only handle
>>> 	numbers or convenience var as numbers.
>>> 	(check_for_flags): New function.
>>> 	(check_for_flags_vqcs): New function.
>>> 	* cli-utils.h (check_for_flags): New function.
>>> 	(check_for_flags_vqcs): New function.
>>
>> I'm not super happy with this design, because first it is
>> still at places biased toward "flags" instead of "command
>> options", and most importantly because it doesn't seem to
>> make it easy to add further options to commands that
>> use check_for_flags_vqcs, without the artificial limitation
>> of requiring all vqcs flags specified together.  E.g., what if we want to
>> add an option like "-foo NUM" to "thread apply" for example.
> 
> Yes I agree, the current way to give the vqcs option is too unflexible,
> so I will rework based on the iterative function you suggest below.
> 
> Just one clarification: I assume that by 'at places biased toward "flags"',
> you mean that the names should be 'check_for_options_vqcs' ?
> Otherwise, can you explain what you mean with bias ?

Yeah, on second thought I think using "flags" for the function is
fine since it only handles flag-like options.  On my option-revamping
prototype I was calling those kind of options "switches" but I never
like that name.  "flags" sounds more usual.  So my concern is more
with the user-visible aspects.

>> maybe even interleave the number-or-range parsing
>> in that loop.
> Probably that can be done, but isn't this a little bit cumbersome ?

Maybe.

> E.g. it means the help 
>   thread apply ID... [OPTIONS] COMMAND
> will become something like
>   thread apply OPTIONS_OR_ID... COMMAND
> and then we have to explain what OPTIONS_OR_ID can be.

I guess we could also say that there's only one "thread apply"
command, and that is looks like this:

 thread apply [OPTION]... [ID... | all] COMMAND

and then specifying options after the ID list works
just because we don't care about order of optional vs
non-optional arguments.

But yeah, there's no need to go there.  Options after
ID is fine with me.

> 
> (and I guess such syntax might make the 'generalised option parser'
> more difficult to implement/use : we better keep ID... as a 
> 'positional argument' for an easy conversion to an generalised
> option/arg parser).
> 
> So, I am more keen to keep
>    thread apply ID... [OPTIONS] COMMAND
> (note: we can always change it in a backward compatible
> way in the future if we really believe mixing OPTIONS and ID...
> has a strong value).

Agreed.

> 
> +  res = check_for_flags (str, flags, flags_counts);
>>> +  if (res == 0)
>>> +    return 0;
>>> +  if (res == -1)
>>> +    error (_("%s only accepts flags %s given individually"),
>>> +	   which_command, flags);
>>
>> I think this error message might look a bit odd.  What does
>> it really mean?
> It catches the below erroneous case, but probably this will become
> 'unknown option -vc' when I do the iterative design:
>   (gdb) thread apply all -vc p 1
>   thread apply all only accepts flags vqcs given individually
>   (gdb) 
> 
> (I think that the generalised option/arg parser you have prototyped
> will be a nice help to have a consistent and easier to code
> gdb command parsing :).

I hope so.  :-)

Thanks,
Pedro Alves
  

On Fri, 2018-06-15 at 17:24 +0100, Pedro Alves wrote:
> Yeah, on second thought I think using "flags" for the function is
> fine since it only handles flag-like options.  On my option-revamping
> prototype I was calling those kind of options "switches" but I never
> like that name.  "flags" sounds more usual.  So my concern is more
> with the user-visible aspects.
I have just send RFA_v3.
Note that following the above discussion, I have kept the word
'flag' for the cases where the code and/or documentation are
only related to flags, and have no other type of arguments.
The wording can be changed whenever other types of options
are added and/or when arg parsing is converted to the
generalized option parser

Thanks for the review comments and suggestions ...

Philippe