Re: [PATCH] gdb: redo whitespace-stripping from commands

[Guinevere Larsen <guinevere@redhat.com>]

Hi Kartik!

I have one minor wording suggestion in the commit message, but otherwise 
I think this change is great!
On 5/31/25 10:17 PM, Kartik K. Agaram wrote:
> Before this patch, trailing whitespace was not stripped from 'set' and
> 'complete' commands. Now I've added 'with' commands to that list because
> they contain a 'complete' subcommand. To accomplish this, I'm fixing an

I'd word it as "I've added 'with' to that list of commands because it 
may contain one of those commands".

Feel free to add my review trailer to the end of your commit message, 
and I hope this gets approved by a global maintainer soon!

Reviewed-By: Guinevere Larsen <guinevere@redhat.com>

-- 
Cheers,
Guinevere Larsen
She/Her/Hers

> ancient TODO to provide a per-command flag controlling
> whitespace-stripping.
>
> Any subcommands of 'with' besides 'complete' or 'set' will continue to
> work because they go through a recursive call to execute_command. The
> outer call for 'with' won't strip whitespace, then the recursive call
> will.
>
> I've also cleaned up 2 obsolete TODOs and NOTEs in related code.
>
> Tested on x86_64-linux.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29784
> ---
>   gdb/cli/cli-cmds.c   | 11 ++++-------
>   gdb/cli/cli-cmds.h   |  4 ----
>   gdb/cli/cli-decode.c |  1 +
>   gdb/cli/cli-decode.h |  8 ++++----
>   gdb/top.c            | 12 +-----------
>   5 files changed, 10 insertions(+), 26 deletions(-)
>
> diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
> index 9a5021f9d08..f2d6d576cb8 100644
> --- a/gdb/cli/cli-cmds.c
> +++ b/gdb/cli/cli-cmds.c
> @@ -427,12 +427,6 @@ complete_command (const char *arg, int from_tty)
>       }
>   }
>   
> -int
> -is_complete_command (struct cmd_list_element *c)
> -{
> -  return cmd_simple_func_eq (c, complete_command);
> -}
> -
>   static void
>   show_version (const char *args, int from_tty)
>   {
> @@ -2702,8 +2696,10 @@ Generic command for showing things about the program being debugged."),
>     add_com_alias ("i", info_cmd, class_info, 1);
>     add_com_alias ("inf", info_cmd, class_info, 1);
>   
> -  add_com ("complete", class_obscure, complete_command,
> +  cmd_list_element *complete_cmd
> +    = add_com ("complete", class_obscure, complete_command,
>   	   _("List the completions for the rest of the line as a command."));
> +  complete_cmd->strip_trailing_white_space_p = false;
>   
>     c = add_show_prefix_cmd ("show", class_info, _("\
>   Generic command for showing things about the debugger."),
> @@ -2726,6 +2722,7 @@ E.g.:\n\
>   You can change multiple settings using nested with, and use\n\
>   abbreviations for commands and/or values.  E.g.:\n\
>     w la p -- w p el u -- p obj"));
> +  with_cmd->strip_trailing_white_space_p = false;
>     set_cmd_completer_handle_brkchars (with_cmd, with_command_completer);
>     add_com_alias ("w", with_cmd, class_vars, 1);
>   
> diff --git a/gdb/cli/cli-cmds.h b/gdb/cli/cli-cmds.h
> index 33d13fb8563..c3004a8e857 100644
> --- a/gdb/cli/cli-cmds.h
> +++ b/gdb/cli/cli-cmds.h
> @@ -149,10 +149,6 @@ extern struct cmd_list_element *showsourcelist;
>   
>   extern unsigned int max_user_call_depth;
>   
> -/* Exported to gdb/top.c */
> -
> -int is_complete_command (struct cmd_list_element *cmd);
> -
>   /* Exported to gdb/main.c */
>   
>   extern void cd_command (const char *, int);
> diff --git a/gdb/cli/cli-decode.c b/gdb/cli/cli-decode.c
> index 48a34667c37..782d38f781f 100644
> --- a/gdb/cli/cli-decode.c
> +++ b/gdb/cli/cli-decode.c
> @@ -582,6 +582,7 @@ add_setshow_cmd_full_erased (const char *name,
>   			     extra_literals, args,
>   			     full_set_doc.release (), set_list);
>     set->doc_allocated = 1;
> +  set->strip_trailing_white_space_p = false;
>   
>     if (set_func != NULL)
>       set->func = set_func;
> diff --git a/gdb/cli/cli-decode.h b/gdb/cli/cli-decode.h
> index 9be446fb641..d3d512adcd4 100644
> --- a/gdb/cli/cli-decode.h
> +++ b/gdb/cli/cli-decode.h
> @@ -60,6 +60,7 @@ struct cmd_list_element
>         allow_unknown (0),
>         abbrev_flag (0),
>         type (not_set_cmd),
> +      strip_trailing_white_space_p (true),
>         doc (doc_)
>     {
>       gdb_assert (name != nullptr);
> @@ -175,11 +176,10 @@ struct cmd_list_element
>        or "show").  */
>     ENUM_BITFIELD (cmd_types) type : 2;
>   
> +  bool strip_trailing_white_space_p;
> +
>     /* Function definition of this command.  NULL for command class
> -     names and for help topics that are not really commands.  NOTE:
> -     cagney/2002-02-02: This function signature is evolving.  For
> -     the moment suggest sticking with either set_cmd_cfunc() or
> -     set_cmd_sfunc().  */
> +     names and for help topics that are not really commands.  */
>     cmd_func_ftype *func;
>   
>     /* The command's real callback.  At present func() bounces through
> diff --git a/gdb/top.c b/gdb/top.c
> index 6adef467b90..8a1e586b4c1 100644
> --- a/gdb/top.c
> +++ b/gdb/top.c
> @@ -503,20 +503,10 @@ execute_command (const char *p, int from_tty)
>   	  arg = *p == '\0' ? nullptr : p;
>   	}
>   
> -      /* FIXME: cagney/2002-02-02: The c->type test is pretty dodgy
> -	 while the is_complete_command(cfunc) test is just plain
> -	 bogus.  They should both be replaced by a test of the form
> -	 c->strip_trailing_white_space_p.  */
> -      /* NOTE: cagney/2002-02-02: The function.cfunc in the below
> -	 can't be replaced with func.  This is because it is the
> -	 cfunc, and not the func, that has the value that the
> -	 is_complete_command hack is testing for.  */
>         /* Clear off trailing whitespace, except for set and complete
>   	 command.  */
>         std::string without_whitespace;
> -      if (arg
> -	  && c->type != set_cmd
> -	  && !is_complete_command (c))
> +      if (arg && c->strip_trailing_white_space_p)
>   	{
>   	  const char *old_end = arg + strlen (arg) - 1;
>   	  p = old_end;