Re: [RFC 1/5] Add helper functions check_for_flags and check_for_flags_vqcs

[Simon Marchi <simark at simark dot ca>]

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