From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 74856 invoked by alias); 23 Sep 2018 20:16:40 -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 74828 invoked by uid 89); 23 Sep 2018 20:16:38 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-2.6 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 spammy=UD:be, auditing, whitespace, tend X-HELO: mailsec102.isp.belgacom.be Received: from mailsec102.isp.belgacom.be (HELO mailsec102.isp.belgacom.be) (195.238.20.98) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sun, 23 Sep 2018 20:16:34 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=skynet.be; i=@skynet.be; q=dns/txt; s=securemail; t=1537733795; x=1569269795; h=message-id:subject:from:to:cc:date:in-reply-to: references:mime-version:content-transfer-encoding; bh=6QHDyUim0KhsbDfcmSyx+i642djyd15QPS62bqKCLrI=; b=CLEARWPvJYoN/SQ/qS4oSXG8gLzoeFbPYcxdW0zeQ28ow9cBMPgjruuh COXxo+CjaaQfbymgT8zr8AqesTpQpA==; Received: from 148.211-243-81.adsl-dyn.isp.belgacom.be (HELO md) ([81.243.211.148]) by relay.skynet.be with ESMTP/TLS/AES256-GCM-SHA384; 23 Sep 2018 22:16:31 +0200 Message-ID: <1537733790.3924.13.camel@skynet.be> Subject: Re: [RFAv2 1/6] New cli-utils.h/.c function extract_info_print_args From: Philippe Waroquiers To: Tom Tromey Cc: gdb-patches@sourceware.org Date: Sun, 23 Sep 2018 20:16:00 -0000 In-Reply-To: <878t3yu8vv.fsf@tromey.com> References: <20180826165359.1600-1-philippe.waroquiers@skynet.be> <20180826165359.1600-2-philippe.waroquiers@skynet.be> <878t3yu8vv.fsf@tromey.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2018-09/txt/msg00780.txt.bz2 Thanks for the comments, feedback below. On Tue, 2018-09-18 at 09:23 -0600, Tom Tromey wrote: > > > > > > "Philippe" == Philippe Waroquiers writes: > > Philippe> cli-utils.c has a new static function extract_arg_maybe_quoted > Philippe> that extracts an argument, possibly single quoted. > > I think it would be better if this handled quotes the same way that > gdb_argv (aka libiberty's buildargv) does. That approach seems ok-ish > (maybe not perfect), and following it would mean that at least we're not > increasing the number of ways that command lines are quoted in gdb. Agreed. In RFAv3, the quoting is handled the same way as gdb_argv. > > I guess the reason you did not simply use gdb_argv is that the trailing > regexp is the rest of the input string. Effectively. Using gdb_argv for handling the list of arguments would create backward incompatibilities (more details below). > > Philippe> +/* See documentation in cli-utils.h. */ > Philippe> + > Philippe> +void > Philippe> +extract_info_print_args (const char* command, > Philippe> + const char **args, > Philippe> + bool *quiet, > Philippe> + std::string *regexp, > Philippe> + std::string *t_regexp) > > I tend to think this should go somewhere else, since cli-utils is more > like "low level" parsing helpers. But I don't really know where I > guess. Yes, I found no better (existing) place, so I kept it there. If you prefer, I can create cli-arg_utils.h for 'higher level' arg parsing helpers if you believe this is cleaner. > > Philippe> + if (*args != NULL && **args != '\000') > Philippe> + *regexp = extract_arg (args); > > It seems to me that this should just be "*regexp = args" for backward > compatibility. This formulation will stop at whitespace, whereas the > previous code did not. Good catch ! I have fixed the regression, and added a specific check in info_qt.exp to verify that a space does not terminate the NAMEREGEXP. > > If changing the meaning of "info types regexp" where the regexp has > whitespace is ok, then switching fully to gdb_argv would be better. I > don't know if it is ok to do but I suspect not. I looked at using gdb_argv, but concluded that using gdb_argv would be backward incompatible for some likely use cases. E.g. in Ada, a . can be part of a variable name, and the behaviour of 'info variables a\.t' would change : a lot more variables would match. More generally, any backslashing of REGEXP special char would be broken. So, I did not switch fully to gdb_argv, but made extract_arg_maybe_quoted quoting and escaping compatible with gdb_argv. > > Philippe> +/* A helper function to extract an argument from *ARG. An argument is > Philippe> + delimited by whitespace, but it can also be optionally quoted using > Philippe> + single quote characters. The return value is empty if no argument > Philippe> + was found. */ > Philippe> + > Philippe> +static std::string > Philippe> +extract_arg_maybe_quoted (const char **arg) > Philippe> +{ > > One question I have is whether we could just change extract_arg to do > the dequoting. Auditing the existing calls to see if this change would > negatively impact them might not be too hard. And, it would be nice to > make gdb's CLI a bit more regular. I examined the calls to extract_arg : I found one possible unlikely backward incompatibility related to single quote handling : the mem command accepts expressions for addresses, and expressions in Ada can contain single quotes. So, it looks possible to have extract_arg behaviour replaced by extract_arg_maybe_quoted behaviour. If you agree with the above analysis, I will work on that in a separate patch series. > > Philippe> +#define INFO_PRINT_ARGS_HELP(entity_kind) \ > Philippe> +"If NAMEREGEXP is provided, only prints the " entity_kind " whose name\n\ > Philippe> +matches NAMEREGEXP.\n\ > Philippe> +If -t TYPEREGEXP is provided, only prints the " entity_kind " whose type\n\ > Philippe> +matches TYPEREGEXP. Note that the matching is done with the type\n\ > Philippe> +printed by the 'whatis' command.\n\ > Philippe> +By default, the command might produce headers and/or messages indicating\n\ > Philippe> +why no " entity_kind " can be printed.\n\ > Philippe> +The flag -q disables the production of these headers and messages." > > Constructing help text like this is i18n-unfriendly. gdb tries to do > this correctly, though IIRC the build isn't all wired up properly and > there aren't any actual translations :-/ > > Maybe it can be reworded somehow. I'm not sure if xgettext can handle > this kind of string concatenation in combination with macro expansion, > either. I have replaced the macro and string concatenation by a function, so that all string constants use the _("...") layout. Thanks Philippe