Re: [RFAv3 1/5] New cli-utils.h/.c function extract_info_print_args

[Pedro Alves <palves at redhat dot com>]

Hi Philippe,

Finally managed to read through the series.  

I have a few comments throughout the series, but nothing very serious.
Probably the next iteration will be good to go.
Apologies for the delay... :-/

On 09/23/2018 10:42 PM, Philippe Waroquiers wrote:
> New cli-utils.h/.c function extract_info_print_args factorises
> the extraction of the args '[-q] [-t TYPEREGEXP] [NAMEREGEXP]'.
> 
> This function will be used by the commands
>   info [args|functions|locals|variables]
> 
> As this function will be used for 'info functions|variables' which
> already have the NAMEREGEXP arg, it provides a backward compatible
> behaviour.
> 
> cli-utils.c has a new static function extract_arg_maybe_quoted
> that extracts an argument, possibly quoted. The behaviour of this
> function is similar to the parsing done by gdb_argv.
> 
> gdb/ChangeLog
> 2018-09-23  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
> 
> 	* cli-utils.c (extract_arg_maybe_quoted): New function.
> 	(extract_info_print_args): New function.
> 	(info_print_args_help): New function.
> 	* cli-utils.h (extract_arg_maybe_quoted): New function.
> 	(extract_info_print_args): New function.
> 	(info_print_args_help): New function.
> ---
>  gdb/cli/cli-utils.c | 159 ++++++++++++++++++++++++++++++++++++++++++++
>  gdb/cli/cli-utils.h |  15 +++++
>  2 files changed, 174 insertions(+)
> 
> diff --git a/gdb/cli/cli-utils.c b/gdb/cli/cli-utils.c
> index 98b7414991..ea4b4b6f61 100644
> --- a/gdb/cli/cli-utils.c
> +++ b/gdb/cli/cli-utils.c
> @@ -23,6 +23,8 @@
>  
>  #include <ctype.h>
>  
> +static std::string extract_arg_maybe_quoted (const char **arg);
> +
>  /* See documentation in cli-utils.h.  */
>  
>  int
> @@ -125,6 +127,84 @@ get_number (char **pp)
>    return result;
>  }
>  
> +/* See documentation in cli-utils.h.  */
> +
> +void
> +extract_info_print_args (const char* command,


'*' should lean against "command":

  "const char *command"

It seems like this function has the same issue that
a function in the "frame apply" series had.  Namely,
that it isn't fit for iterative use, so that some command
using it could support other options.  Would it be possible
to adjust this in that direction?

> +			 const char **args,
> +			 bool *quiet,
> +			 std::string *regexp,
> +			 std::string *t_regexp)
> +{
> +  *quiet = false;
> +  *regexp = "";
> +  *t_regexp = "";
> +
> +  while (*args != NULL)
> +    {
> +      if (check_for_argument (args, "--", 2))
> +	{
> +	  *args = skip_spaces (*args);
> +	  break;
> +	}
> +
> +      if (check_for_argument (args, "-t", 2))
> +	{
> +	  *t_regexp = extract_arg_maybe_quoted (args);
> +	  *args = skip_spaces (*args);
> +	  continue;
> +	}
> +
> +      if (check_for_argument (args, "-q", 2))
> +	{
> +	  *quiet = true;
> +	  *args = skip_spaces (*args);
> +	  continue;
> +	}
> +
> +      if (**args != '-')
> +	break;
> +
> +      std::string option = extract_arg (args);
> +      error (_("Unrecognized option '%s' to %s command.  "
> +	       "Try \"help %s\"."), option.c_str (),
> +	     command, command);
> +    }
> +
> +  if (*args != NULL && **args != '\000')
> +    *regexp = *args;
> +

Spurious empty line.


> +}
> +
> +/* See documentation in cli-utils.h.  */
> +
> +const char*
> +info_print_args_help (const char* prefix,
> +		      const char* entity_kind)

Formatting, space on the left of '*'.

> +{
> +  std::string h;
> +
> +  h = prefix;
> +  h += _("If NAMEREGEXP is provided, only prints the ");
> +  h += entity_kind;
> +  h +=_(" whose name\n\
> +matches NAMEREGEXP.\n\
> +If -t TYPEREGEXP is provided, only prints the ");
> +  h += entity_kind;
> +  h +=_(" whose type\n\
> +matches TYPEREGEXP.  Note that the matching is done with the type\n\
> +printed by the 'whatis' command.\n\
> +By default, the command might produce headers and/or messages indicating\n\
> +why no ");
> +  h += entity_kind;
> +  h += _(" can be printed.\n\
> +The flag -q disables the production of these headers and messages.");
> +

Building a string in chunks like this is not very i18n-friendly,
because depending on the language, the position of the variables
may differ.  Also, harder for the translator to see how it all
fits together.  It's better to write it all as one string, and
use printf-style parameters.  It's also more readable in the code.
While at it, why return a copy of the string, instead of returning
the std::string directly?

Like so:

std::string
info_print_args_help (const char *prefix,
                      const char *entity_kind)
{
  return string_printf (_("\
%sIf NAMEREGEXP is provided, only prints the %s whose name\n\
matches NAMEREGEXP.\n\
If -t TYPEREGEXP is provided, only prints the %s whose type\n\
matches TYPEREGEXP.  Note that the matching is done with the type\n\
printed by the 'whatis' command.\n\
By default, the command might produce headers and/or messages indicating\n\
why no %s can be printed.\n\
The flag -q disables the production of these headers and messages."),
                       prefix, entity_kind, entity_kind, entity_kind);
}

Still not perfect, because the different uses of entity_kind
could end up translating differently (e.g., singular vs plural),
but better than before, I think.

> +  return xstrdup (h.c_str ());
> +}
> +
> +
> +
>  /* See documentation in cli-utils.h.  */
>  
>  number_or_range_parser::number_or_range_parser (const char *string)
> @@ -282,6 +362,85 @@ remove_trailing_whitespace (const char *start, const char *s)
>    return s;
>  }
>  
> +/* A helper function to extract an argument from *ARG.  An argument is
> +   delimited by whitespace, but it can also be optionally quoted.
> +   The quoting and special characters are handled similarly to
> +   the parsing done by gdb_argv.
> +   The return value is empty if no argument was found.  */
> +
> +static std::string
> +extract_arg_maybe_quoted (const char **arg)
> +{
> +  int squote = 0;
> +  int dquote = 0;
> +  int bsquote = 0;

Use bool and true/false throughout the function.

> +  std::string result = std::string ();

Just write:

   std::string result;

I'd suggest doing:

 const char *p = *arg;

then work with p throughout the function, and then do
at the end:

 *arg = p;

That would avoid the double '**' throughout.


> +
> +  /* Find the start of the argument.  */
> +  *arg = skip_spaces (*arg);
> +
> +  /* Parse *arg similarly to gdb_argv buildargv function.  */
> +  while (**arg != '\0')
> +    {
> +      if (isspace (**arg) && !squote && !dquote && !bsquote)
> +	{
> +	  break;
> +	}
> +      else
> +	{
> +	  if (bsquote)
> +	    {
> +	      bsquote = 0;
> +	      result += **arg;
> +	    }
> +	  else if (**arg == '\\')
> +	    {
> +	      bsquote = 1;
> +	    }
> +	  else if (squote)
> +	    {
> +	      if (**arg == '\'')
> +		{
> +		  squote = 0;
> +		}
> +	      else
> +		{
> +		  result += **arg;
> +		}

Drop the curly braces around a single line expression
(throughout):

      if (**arg == '\'')
        squote = 0;
      else
        result += **arg;


> +	    }
> +	  else if (dquote)
> +	    {
> +	      if (**arg == '"')
> +		{
> +		  dquote = 0;
> +		}
> +	      else
> +		{
> +		  result += **arg;
> +		}
> +	    }
> +	  else
> +	    {
> +	      if (**arg == '\'')
> +		{
> +		  squote = 1;
> +		}
> +	      else if (**arg == '"')
> +		{
> +		  dquote = 1;
> +		}
> +	      else
> +		{
> +		  result += **arg;
> +		}
> +	    }
> +	  (*arg)++;
> +	}
> +    }
> +
> +  return result;
> +}
> +
>  /* See documentation in cli-utils.h.  */
>  
>  std::string
> diff --git a/gdb/cli/cli-utils.h b/gdb/cli/cli-utils.h
> index fa7d02d719..8ce8db1860 100644
> --- a/gdb/cli/cli-utils.h
> +++ b/gdb/cli/cli-utils.h
> @@ -39,6 +39,21 @@ extern int get_number (const char **);
>  
>  extern int get_number (char **);
>  
> +/* Extract from args the arguments [-q] [-t TYPEREGEXP] [--] NAMEREGEXP.

"from ARGS the"

> +   Only the above flags are accepted.  Any other flag will raise an error.
> +   COMMAND is used to produce the error message if an invalid flag is
> +   given.  */
> +extern void extract_info_print_args (const char* command,

"const char *command"

> +				     const char **args,
> +				     bool *quiet,
> +				     std::string *regexp,
> +				     std::string *t_regexp);
> +
> +/* Builds the help string for a command documented by PREFIX,
> +   followed by the extract_info_print_args help for ENTITY_KIND.  */
> +extern const char* info_print_args_help (const char* prefix,
> +					 const char* entity_kind);

space before *, and not after:

  "const char *info_print_args_help"

> +
>  /* Parse a number or a range.
>     A number will be of the form handled by get_number.
>     A range will be of the form <number1> - <number2>, and
> 

Thanks,
Pedro Alves