From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 120477 invoked by alias); 18 May 2018 01:22:23 -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 120462 invoked by uid 89); 18 May 2018 01:22:23 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 spammy=mutually, solo, accepts, HTo:D*be X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 18 May 2018 01:22:21 +0000 Received: from [10.0.0.11] (unknown [192.222.164.54]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id BF8DA1E512; Thu, 17 May 2018 21:22:18 -0400 (EDT) Subject: Re: [RFC 1/5] Add helper functions check_for_flags and check_for_flags_vqcs To: Philippe Waroquiers , gdb-patches@sourceware.org References: <20180505192804.12731-1-philippe.waroquiers@skynet.be> <20180505192804.12731-2-philippe.waroquiers@skynet.be> From: Simon Marchi Message-ID: <0a630fc9-18bd-6d3b-5558-9a0389c64d4f@simark.ca> Date: Fri, 18 May 2018 01:56:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <20180505192804.12731-2-philippe.waroquiers@skynet.be> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2018-05/txt/msg00395.txt.bz2 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 > - 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