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

Simon Marchi
2018-05-18T01:22:23Z

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