[RFC,1/5] 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.

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... argument to
thread apply ID... [-FLAGS...] COMMAND

Modify gdb/testsuite/gdb.multi/tids.exp, as the gdb error message
for 'thread apply -$unknownconvvar p 1'
is now the more clear:
  Invalid thread ID: -$unknownconvvar p 1
instead of previously:
  negative value
---
 gdb/cli/cli-utils.c              | 116 ++++++++++++++++++++++++++++++++++++++-
 gdb/cli/cli-utils.h              |  30 ++++++++++
 gdb/testsuite/gdb.multi/tids.exp |   4 +-
 3 files changed, 145 insertions(+), 5 deletions(-)
  

Hi Philippe,

I don't see the full picture yet just with this patch, so here is a
first pass of comments, more nits than anything else.

On 2018-05-05 03:28 PM, Philippe Waroquiers wrote:
> check_for_flags helper function allows to look for a set of flags at
> the start of a string.
> 
> 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... argument to
> thread apply ID... [-FLAGS...] COMMAND
> 
> Modify gdb/testsuite/gdb.multi/tids.exp, as the gdb error message
> for 'thread apply -$unknownconvvar p 1'
> is now the more clear:
>   Invalid thread ID: -$unknownconvvar p 1
> instead of previously:
>   negative value
> ---
>  gdb/cli/cli-utils.c              | 116 ++++++++++++++++++++++++++++++++++++++-
>  gdb/cli/cli-utils.h              |  30 ++++++++++
>  gdb/testsuite/gdb.multi/tids.exp |   4 +-
>  3 files changed, 145 insertions(+), 5 deletions(-)
> 
> diff --git a/gdb/cli/cli-utils.c b/gdb/cli/cli-utils.c
> index c55b5035e4..4abd501d52 100644
> --- a/gdb/cli/cli-utils.c
> +++ b/gdb/cli/cli-utils.c
> @@ -22,7 +22,6 @@
>  #include "value.h"
>  
>  #include <ctype.h>
> -

Unintended change?

>  /* See documentation in cli-utils.h.  */
>  
>  int
> @@ -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

Capital "i"

> +	 of flags. So, do not parse a range if the - is followed by
> +	 an alpha.  */
> +      if (*m_cur_tok == '-' && !isalpha(*(m_cur_tok+1)))

Missing space after isalpha, spaces around +.  It could also be written
m_cur_token[1].

Could you add some simple unit tests in the unittests/ directory for
number_or_range_parser (I don't think there are any so far)?  It will
help illustrate your use case.

>  	{
>  	  const char **temp;
>  
> @@ -196,7 +198,10 @@ number_or_range_parser::get_number ()
>  	}
>      }
>    else
> -    error (_("negative value"));
> +    {
> +      if (*m_cur_tok >= '0' && *m_cur_tok <= '9')

Use isdigit?

> +	error (_("negative value"));
> +    }
>    m_finished = *m_cur_tok == '\0';
>    return m_last_retval;
>  }
> @@ -304,3 +309,108 @@ check_for_argument (const char **str, const char *arg, int arg_len)
>      }
>    return 0;
>  }
> +

Add a

  /* See cli-utils.h.  */

> +int
> +check_for_flags (const char **str, const char *flags,
> +		 int *flags_counts)
> +{
> +  const char *p = *str;
> +  bool all_valid = true;
> +
> +  /* First set the flags_counts to 0.  */
> +  {
> +    const char *f = flags;
> +    while (*f)
> +      {
> +	flags_counts[f - flags] = 0;
> +	f++;
> +      }
> +  }

What about something like

  memset (flags_count, 0, sizeof (flags_count[0]) * strlen (flags));

> +
> +  if (*p != '-')
> +    return 0;
> +
> +  p++;
> +  /* If - is the last char, or is followed by a space or a $, then
> +     we do not have flags.  */
> +  if (*p == '\0' || (isspace (*p) || *p == '$'))

You can remove some parenthesis:

  if (*p == '\0' || isspace (*p) || *p == '$')

But could we be more restrictive here and only allow isalpha characters?

  if (!isalpha (*p))
    return 0;

> +    return 0;
> +
> +  /* We might have a command that accepts optional flags followed by
> +     a negative integer. So, do not handle a negative integer as invalid
> +     flags : rather let the caller handle the negative integer.  */
> +  {
> +    const char *p1 = p;
> +    while (*p1 >= '0' && *p1 <= '9')

isdigit?

> +      ++p1;
> +    if (p != p1 && (*p1 == '\0' || isspace (*p1)))
> +      return 0;
> +  }
> +
> +  /* We have a set of flags :
> +     Scan and validate the flags, and update flags_counts for valid flags.  */
> +  while (*p != '\0' && !isspace (*p))
> +    {
> +      const char *f = flags;
> +      bool valid = false;
> +
> +      while (*f && !valid)

*f != nullptr

If I understand the algorithm right, I think it is not necessary to check for
!valid here.

> +	{
> +	  valid = *f == *p;
> +	  if (valid)
> +	    {
> +	      flags_counts[f - flags]++;
> +	      break;
> +	    }
> +	  f++;
> +	}
> +      all_valid = all_valid && valid;
> +      p++;
> +    }
> +
> +  /* Skip the space(s) */
> +  while (*p && isspace((int) *p))

Space after isspace, but it could probably use the skip_spaces function.

> +    ++p;
> +
> +  if (all_valid)
> +    {
> +      *str = p;
> +      return 1;
> +    }
> +  else
> +    return -1;
> +}
> +

/* See 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[4];

Maybe use strlen (flags) instead of the literal 4?

> +  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"), which_command, flags);
> +  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 does not accept at the same time flags c and s"),
> +	   which_command);

Another common way to say this would be

  ""%s: flags c and s are mutually exclusive."

Thanks,

Simon
  

On Thu, 2018-05-17 at 21:22 -0400, Simon Marchi wrote:
> > +  /* First set the flags_counts to 0.  */
> > +  {
> > +    const char *f = flags;
> > +    while (*f)
> > +      {
> > +	flags_counts[f - flags] = 0;
> > +	f++;
> > +      }
> > +  }
> 
> What about something like
> 
>   memset (flags_count, 0, sizeof (flags_count[0]) * strlen (flags));

The code initialising the flags_counts is somewhat similar in structure
to the code that increments the flags_counts.

So, it looks more clear to me to have the zero-ing code and
the incrementing code looking like each other.

But if for gdb, using memset is the typical pattern to zero
an array of int, fine for me.

What do you think ?

Philippe
  

On 2018-05-18 05:42 PM, Philippe Waroquiers wrote:
> On Thu, 2018-05-17 at 21:22 -0400, Simon Marchi wrote:
>>> +  /* First set the flags_counts to 0.  */
>>> +  {
>>> +    const char *f = flags;
>>> +    while (*f)
>>> +      {
>>> +	flags_counts[f - flags] = 0;
>>> +	f++;
>>> +      }
>>> +  }
>>
>> What about something like
>>
>>   memset (flags_count, 0, sizeof (flags_count[0]) * strlen (flags));
> 
> The code initialising the flags_counts is somewhat similar in structure
> to the code that increments the flags_counts.
> 
> So, it looks more clear to me to have the zero-ing code and
> the incrementing code looking like each other.
> 
> But if for gdb, using memset is the typical pattern to zero
> an array of int, fine for me.
> 
> What do you think ?

I don't really mind, I just thought the memset was short and clear,
but both work.

Simon
  

On Fri, 2018-05-18 at 19:38 -0400, Simon Marchi wrote:
> I don't really mind, I just thought the memset was short and clear,
> but both work.
Ok, I will use memset (clearly, the usual pattern).

Thanks for the feedback.
I will start working on the comments.

Philippe