Re: [RFAv2 1/6] New cli-utils.h/.c function extract_info_print_args

[Philippe Waroquiers <philippe dot waroquiers at skynet dot be>]

Thanks for the comments, feedback below.

On Tue, 2018-09-18 at 09:23 -0600, Tom Tromey wrote:
> > > > > > "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> 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