From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 74756 invoked by alias); 13 Jun 2018 19:52:41 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 74740 invoked by uid 89); 13 Jun 2018 19:52:40 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.3 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=solo, specialised, sk:number_, sk:get_num X-HELO: mx1.redhat.com Received: from mx3-rdu2.redhat.com (HELO mx1.redhat.com) (66.187.233.73) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 13 Jun 2018 19:52:38 +0000 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 3F8AC40776DD; Wed, 13 Jun 2018 19:52:37 +0000 (UTC) Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id 7686920244E0; Wed, 13 Jun 2018 19:52:36 +0000 (UTC) Subject: Re: [RFA_v2 1/8] Add helper functions check_for_flags and check_for_flags_vqcs To: Philippe Waroquiers , gdb-patches@sourceware.org References: <20180605204905.30612-1-philippe.waroquiers@skynet.be> <20180605204905.30612-2-philippe.waroquiers@skynet.be> From: Pedro Alves Message-ID: Date: Wed, 13 Jun 2018 19:52:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 MIME-Version: 1.0 In-Reply-To: <20180605204905.30612-2-philippe.waroquiers@skynet.be> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2018-06/txt/msg00331.txt.bz2 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 > > * 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