Re: [PATCH 1/4] gdb: add lookup_cmd_exact to simplify a common pattern

[Lancelot SIX via Gdb-patches <gdb-patches@sourceware.org>]

Hi

I just have a few style-related remarks above.

On Fri, Jan 08, 2021 at 10:07:03AM +0000, Marco Barisione via Gdb-patches wrote:
> In code dealing with commands, there's a pattern repeated a few times of
> calling lookup_cmd with some speficic arguments and then using strcmp
> on the returned command to check for an exact match.
> As a later patch would add a few more similar lines of code, this patch
> adds a new lookup_cmd_exact function which simplify this use case.
> 
> gdb/ChangeLog:
> 
> 	* cli/cli-decode.c (lookup_cmd_exact): Add.
> 	* cli/cli-script.c (do_define_command): Use lookup_cmd_exact.
> 	(define_prefix_command): Ditto.
> 	* command.h: Add lookup_cmd_exact.
> ---
>  gdb/cli/cli-decode.c | 15 +++++++++++++++
>  gdb/cli/cli-script.c | 23 ++++++-----------------
>  gdb/command.h        | 19 +++++++++++++++++++
>  3 files changed, 40 insertions(+), 17 deletions(-)
> 
> diff --git a/gdb/cli/cli-decode.c b/gdb/cli/cli-decode.c
> index 99bd4c6d2cd..87785107e3d 100644
> --- a/gdb/cli/cli-decode.c
> +++ b/gdb/cli/cli-decode.c
> @@ -1875,6 +1875,21 @@ lookup_cmd (const char **line, struct cmd_list_element *list,
>    return 0;
>  }
>  
> +/* See command.h.  */
> +
> +struct cmd_list_element *
> +lookup_cmd_exact (const char *name,
> +		  struct cmd_list_element *list,
> +		  bool ignore_help_classes)
> +{
> +  const char *tem = name;
> +  struct cmd_list_element *cmd = lookup_cmd (&tem, list, "", NULL, -1,

Probably s/NULL/nullptr/ ?

> +					     ignore_help_classes);
> +  if (cmd && strcmp (name, cmd->name) != 0)

I think gdb prefers explicit comparison to check for null pointers:

https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Comparison_With_NULL_And_Zero

+  if (cmd != nullptr && strcmp (name, cmd->name) != 0)

BR
Lancelot

> +    cmd = nullptr;
> +  return cmd;
> +}
> +
>  /* We are here presumably because an alias or command in TEXT is
>     deprecated and a warning message should be generated.  This
>     function decodes TEXT and potentially generates a warning message
> diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c
> index 9d0dd7796e0..0544f3efb1b 100644
> --- a/gdb/cli/cli-script.c
> +++ b/gdb/cli/cli-script.c
> @@ -1391,7 +1391,7 @@ do_define_command (const char *comname, int from_tty,
>        CMD_POST_HOOK
>      };
>    struct cmd_list_element *c, *newc, *hookc = 0, **list;
> -  const char *tem, *comfull;
> +  const char *comfull;
>    int  hook_type      = CMD_NO_HOOK;
>    int  hook_name_size = 0;
>     
> @@ -1403,11 +1403,7 @@ do_define_command (const char *comname, int from_tty,
>    comfull = comname;
>    list = validate_comname (&comname);
>  
> -  /* Look it up, and verify that we got an exact match.  */
> -  tem = comname;
> -  c = lookup_cmd (&tem, *list, "", NULL, -1, 1);
> -  if (c && strcmp (comname, c->name) != 0)
> -    c = 0;
> +  c = lookup_cmd_exact (comname, *list);
>  
>    if (c && commands == nullptr)
>      {
> @@ -1448,11 +1444,9 @@ do_define_command (const char *comname, int from_tty,
>  
>    if (hook_type != CMD_NO_HOOK)
>      {
> -      /* Look up cmd it hooks, and verify that we got an exact match.  */
> -      tem = comname + hook_name_size;
> -      hookc = lookup_cmd (&tem, *list, "", NULL, -1, 0);
> -      if (hookc && strcmp (comname + hook_name_size, hookc->name) != 0)
> -	hookc = 0;
> +      /* Look up cmd it hooks.  */
> +      hookc = lookup_cmd_exact (comname + hook_name_size, *list,
> +				/* ignore_help_classes = */ false);
>        if (!hookc && commands == nullptr)
>  	{
>  	  warning (_("Your new `%s' command does not "
> @@ -1593,17 +1587,12 @@ static void
>  define_prefix_command (const char *comname, int from_tty)
>  {
>    struct cmd_list_element *c, **list;
> -  const char *tem;
>    const char *comfull;
>  
>    comfull = comname;
>    list = validate_comname (&comname);
>  
> -  /* Look it up, and verify that we got an exact match.  */
> -  tem = comname;
> -  c = lookup_cmd (&tem, *list, "", NULL, -1, 1);
> -  if (c != nullptr && strcmp (comname, c->name) != 0)
> -    c = nullptr;
> +  c = lookup_cmd_exact (comname, *list);
>  
>    if (c != nullptr && c->theclass != class_user)
>      error (_("Command \"%s\" is built-in."), comfull);
> diff --git a/gdb/command.h b/gdb/command.h
> index 79e5017ff7a..827a19637a2 100644
> --- a/gdb/command.h
> +++ b/gdb/command.h
> @@ -326,6 +326,25 @@ extern struct cmd_list_element *lookup_cmd_1
>  	 struct cmd_list_element **result_list, std::string *default_args,
>  	 int ignore_help_classes, bool lookup_for_completion_p = false);
>  
> +/* Look up the command called NAME in the command list LIST.
> +
> +   Unlike LOOKUP_CMD, partial matches are ignored and only exact matches
> +   on NAME are considered.
> +
> +   LIST is a chain of struct cmd_list_element's.
> +
> +   If IGNORE_HELP_CLASSES is true (the default), ignore any command list
> +   elements which are actually help classes rather than commands (i.e.
> +   the function field of the struct cmd_list_element is null).
> +
> +   If found, return the struct cmd_list_element for that command,
> +   otherwise return NULLPTR.  */
> +
> +extern struct cmd_list_element *lookup_cmd_exact
> +			(const char *name,
> +			 struct cmd_list_element *list,
> +			 bool ignore_help_classes = true);
> +
>  extern struct cmd_list_element *deprecate_cmd (struct cmd_list_element *,
>  					       const char * );
>  
> -- 
> 2.28.0
>