Re: [PATCH 2/2] gdb: Rework command completion on 'tui reg'.

[Pedro Alves <palves at redhat dot com>]

On 05/21/2015 11:17 PM, Andrew Burgess wrote:
> We previously specified a few known register groups for the 'tui reg'
> command.  Other register groups could be accessed, but only by using the
> 'tui reg next' command and cycling through all the groups.
> 
> This commit removes the hard coded sub-commands of 'tui reg' and instead
> adds dynamic completion of sub-commands based on the architecturally
> defined register groups, giving immediate access to all available
> register groups.
> 
> There is still the 'next' and 'prev' commands for cycling through the
> register groups if that's wanted.

I think this deserves a news entry, being a user-visible change.

> gdb/ChangeLog:
> 
> 	* completer.c (reg_or_group_completer_1): New function containing
> 	old reg_or_group_completer.
> 	(reg_or_group_completer): Call new reg_or_group_completer_1.
> 	(reggroup_completer): Call new reg_or_group_completer_1.
> 	* completer.h (reggroup_completer): Add declaration.
> 	* tui/tui-regs.c: Add 'completer.h' include.
> 	(tui_reg_next_command): Renamed to...
> 	(tui_reg_name): ...this.  Adjust parameters.
> 	(tui_reg_prev_command): Renamed to...
> 	(tui_reg_prev): ...this.  Adjust parameters.
> 	(tui_reg_float_command): Delete.
> 	(tui_reg_general_command): Delete.
> 	(tui_reg_system_command): Delete.
> 	(tui_reg_command): Rewrite to perform switching of register group.
> 	(tuireglist): Remove.
> 	(tui_reggroup_completer): New function.
> 	(_initialize_tui_regs): Remove 'tui reg' sub-commands, update
> 	creation of 'tui reg' command.
> ---
>  gdb/ChangeLog      |  21 +++++++++
>  gdb/completer.c    |  57 ++++++++++++++++---------
>  gdb/completer.h    |   3 ++
>  gdb/tui/tui-regs.c | 123 +++++++++++++++++++++++++++++++++--------------------
>  4 files changed, 140 insertions(+), 64 deletions(-)
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index b19e3ca..5ee18d0 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,5 +1,26 @@
>  2015-05-21  Andrew Burgess  <andrew.burgess@embecosm.com>
>  
> +	* completer.c (reg_or_group_completer_1): New function containing
> +	old reg_or_group_completer.
> +	(reg_or_group_completer): Call new reg_or_group_completer_1.
> +	(reggroup_completer): Call new reg_or_group_completer_1.
> +	* completer.h (reggroup_completer): Add declaration.
> +	* tui/tui-regs.c: Add 'completer.h' include.
> +	(tui_reg_next_command): Renamed to...
> +	(tui_reg_name): ...this.  Adjust parameters.
> +	(tui_reg_prev_command): Renamed to...
> +	(tui_reg_prev): ...this.  Adjust parameters.
> +	(tui_reg_float_command): Delete.
> +	(tui_reg_general_command): Delete.
> +	(tui_reg_system_command): Delete.
> +	(tui_reg_command): Rewrite to perform switching of register group.
> +	(tuireglist): Remove.
> +	(tui_reggroup_completer): New function.
> +	(_initialize_tui_regs): Remove 'tui reg' sub-commands, update
> +	creation of 'tui reg' command.
> +
> +2015-05-21  Andrew Burgess  <andrew.burgess@embecosm.com>
> +
>  	* tui/tui-regs.c (tui_reg_prev_command): New function.
>  	(_initialize_tui_regs): Add 'prev' command for 'tui reg'.
>  	* reggroups.c (reggroup_prev): New function.
> diff --git a/gdb/completer.c b/gdb/completer.c
> index c8c0e4c..73f734e 100644
> --- a/gdb/completer.c
> +++ b/gdb/completer.c
> @@ -971,9 +971,10 @@ signal_completer (struct cmd_list_element *ignore,
>  
>  /* Complete on a register or reggroup.  */
>  
> -VEC (char_ptr) *
> -reg_or_group_completer (struct cmd_list_element *ignore,
> -			const char *text, const char *word)
> +static VEC (char_ptr) *
> +reg_or_group_completer_1 (struct cmd_list_element *ignore,
> +			  const char *text, const char *word,
> +			  int add_registers, int add_reggroups)

Making this a bit mask may make callers more obvious.

> +  if (add_registers)
> +    for (i = 0;
> +	 (name = user_reg_map_regnum_to_name (gdbarch, i)) != NULL;
> +	 i++)
> +      {
> +	if (*name != '\0' && strncmp (word, name, len) == 0)
> +	  VEC_safe_push (char_ptr, result, xstrdup (name));
> +      }

Would you mind wrapping the whole for in a {} block?  I think
you could move the 'i' variable to this scope.

> +
> +  if (add_reggroups)
> +    for (group = reggroup_next (gdbarch, NULL);
> +	 group != NULL;
> +	 group = reggroup_next (gdbarch, group))
> +      {
> +	name = reggroup_name (group);
> +	if (strncmp (word, name, len) == 0)
> +	  VEC_safe_push (char_ptr, result, xstrdup (name));
> +      }

Likewise, and move 'group' local.

>  
>    return result;
>  }
>  
> +VEC (char_ptr) *
> +reg_or_group_completer (struct cmd_list_element *ignore,
> +			const char *text, const char *word)
> +{
> +  /* Complete on both registers and reggroups.  */
> +  return reg_or_group_completer_1 (ignore, text, word, 1, 1);
> +}
> +
> +VEC (char_ptr) *
> +reggroup_completer (struct cmd_list_element *ignore,
> +		    const char *text, const char *word)
> +{
> +  /* Complete on reggroups only.  */
> +  return reg_or_group_completer_1 (ignore, text, word, 0, 1);
> +}

Intro comments.  You could reuse the comments inside the
functions outside.

>  
>  /* Get the list of chars that are considered as word breaks
>     for the current command.  */
> diff --git a/gdb/completer.h b/gdb/completer.h
> index 56e1a2b..6c1f257 100644
> --- a/gdb/completer.h
> +++ b/gdb/completer.h
> @@ -96,6 +96,9 @@ extern VEC (char_ptr) *signal_completer (struct cmd_list_element *,
>  extern VEC (char_ptr) *reg_or_group_completer (struct cmd_list_element *,
>  					       const char *, const char *);
>  
> +extern VEC (char_ptr) *reggroup_completer (struct cmd_list_element *,
> +					   const char *, const char *);
> +
>  extern char *get_gdb_completer_quote_characters (void);
>  
>  extern char *gdb_completion_word_break_characters (void);
> diff --git a/gdb/tui/tui-regs.c b/gdb/tui/tui-regs.c
> index 8dbcda1..685a896 100644
> --- a/gdb/tui/tui-regs.c
> +++ b/gdb/tui/tui-regs.c
> @@ -39,6 +39,7 @@
>  #include "tui/tui-io.h"
>  #include "reggroups.h"
>  #include "valprint.h"
> +#include "completer.h"
>  
>  #include "gdb_curses.h"
>  
> @@ -557,10 +558,8 @@ tui_display_register (struct tui_data_element *data,
>  }
>  
>  static void
> -tui_reg_next_command (char *arg, int from_tty)
> +tui_reg_next (struct gdbarch *gdbarch)
>  {
> -  struct gdbarch *gdbarch = get_current_arch ();
> -
>    if (TUI_DATA_WIN != NULL)
>      {
>        struct reggroup *group
> @@ -576,10 +575,8 @@ tui_reg_next_command (char *arg, int from_tty)
>  }
>  
>  static void
> -tui_reg_prev_command (char *arg, int from_tty)
> +tui_reg_prev (struct gdbarch *gdbarch)
>  {
> -  struct gdbarch *gdbarch = get_current_arch ();
> -
>    if (TUI_DATA_WIN != NULL)
>      {
>        struct reggroup *group
> @@ -606,31 +603,84 @@ tui_reg_prev_command (char *arg, int from_tty)
>  }
>  
>  static void
> -tui_reg_float_command (char *arg, int from_tty)
> +tui_reg_command (char *args, int from_tty)
>  {
> -  tui_show_registers (float_reggroup);
> -}
> +  struct gdbarch *gdbarch = get_current_arch ();
>  
> -static void
> -tui_reg_general_command (char *arg, int from_tty)
> -{
> -  tui_show_registers (general_reggroup);
> -}
> +  if (args)

args != NULL


> +    {
> +      if (strcmp (args, "next") == 0)
> +	tui_reg_next (gdbarch);
> +      else if (strcmp (args, "prev") == 0)
> +	tui_reg_prev (gdbarch);

AFAICS, this doesn't handle unambiguious "next", "prev"
abbreviations, though "help tui reg" says it should work.

> +      else
> +	{
> +	  struct reggroup *group = NULL, *match = NULL;
> +	  size_t len = strlen (args);
>  
> -static void
> -tui_reg_system_command (char *arg, int from_tty)
> -{
> -  tui_show_registers (system_reggroup);
> +	  for (group = reggroup_next (gdbarch, group);
> +	       group != NULL;
> +	       group = reggroup_next (gdbarch, group))
> +	    {
> +	      if (strncmp (reggroup_name (group), args, len) == 0)
> +		{
> +		  if (match != NULL)
> +		    {
> +		      match = NULL;
> +		      break;

IIUC, this is to handle ambiguous cases, right?

> +		    }
> +		  match = group;

> +		}
> +	    }
> +
> +	  if (match != NULL)
> +	    tui_show_registers (match);
> +	  else
> +	    error (_("unknown register group '%s'"), args);

Though I think the ambiguous case should have a different error.

> +	}
> +    }
> +  else
> +    {
> +      struct reggroup *group = NULL;

No need to initialize as NULL ...

> +
> +      printf_unfiltered (_("\"tui reg\" must be followed by the name of "
> +			   "either a register group,\nor one of 'next' "
> +			   "or 'prev'.  Known register groups are:\n"));
> +      for (group = reggroup_next (gdbarch, group); group != NULL; )

... with:

      for (group = reggroup_next (gdbarch, NULL); group != NULL; )

which is a bit clearer.

> +	{
> +	  printf_unfiltered ("%s", reggroup_name (group));
> +	  group = reggroup_next (gdbarch, group);
> +	  if (group != NULL)
> +	    printf_unfiltered (", ");
> +	}
> +      printf_unfiltered ("\n");

(Though IMO, something like this is simpler to follow / more obvious:

      int first = 1;

      for (group = reggroup_next (gdbarch, NULL);
           group != NULL;
           group = reggroup_next (gdbarch, group))
	{
	  if (!first)
	    printf_unfiltered (", ");
          else
            first = 0;
	  printf_unfiltered ("%s", reggroup_name (group));
	}
)

> +    }
>  }
>  
> -static struct cmd_list_element *tuireglist;
> +/* Complete names of register groups, and add the special "prev" and "next"
> +   names.  */
>  
> -static void
> -tui_reg_command (char *args, int from_tty)
> +static VEC (char_ptr) *
> +tui_reggroup_completer (struct cmd_list_element *ignore,
> +			const char *text, const char *word)
>  {
> -  printf_unfiltered (_("\"tui reg\" must be followed by the name of a "
> -                     "tui reg command.\n"));
> -  help_list (tuireglist, "tui reg ", all_commands, gdb_stdout);
> +  VEC (char_ptr) *result = NULL;
> +  static const char *extra[] = { "next", "prev", NULL };
> +  size_t len = strlen (word);
> +  const char **tmp;
> +
> +  if (!target_has_registers)
> +    return result;

Do we need this check?  If the target is not running yet, we still
have a default architecture.  AFAICS, tui_reg_command itself works
with get_current_arch, which works even without a running program.
I think the completer should match.  Ah, reggroup_completer gets
gdbarch from the selected frame.  Definitely the completer and the
"tui reg" function itself should use the same gdbarch.  Sounds like
we should pass a gdbarch to reggroup_completer, and leave it to
callers to decide which to use.

> +
> +  result = reggroup_completer (ignore, text, word);
> +
> +  for (tmp = extra; *tmp; ++tmp)

*tmp != NULL

> +    {
> +      if (strncmp (word, *tmp, len) == 0)
> +	VEC_safe_push (char_ptr, result, xstrdup (*tmp));
> +    }
> +
> +  return result;
>  }
>  
>  /* Provide a prototype to silence -Wmissing-prototypes.  */
> @@ -639,30 +689,13 @@ extern initialize_file_ftype _initialize_tui_regs;
>  void
>  _initialize_tui_regs (void)
>  {
> -  struct cmd_list_element **tuicmd;
> +  struct cmd_list_element **tuicmd, *cmd;
>  
>    tuicmd = tui_get_cmd_list ();
>  
> -  add_prefix_cmd ("reg", class_tui, tui_reg_command,
> -                  _("TUI commands to control the register window."),
> -                  &tuireglist, "tui reg ", 0,
> -                  tuicmd);
> -
> -  add_cmd ("float", class_tui, tui_reg_float_command,
> -           _("Display only floating point registers."),
> -           &tuireglist);
> -  add_cmd ("general", class_tui, tui_reg_general_command,
> -           _("Display only general registers."),
> -           &tuireglist);
> -  add_cmd ("system", class_tui, tui_reg_system_command,
> -           _("Display only system registers."),
> -           &tuireglist);
> -  add_cmd ("next", class_tui, tui_reg_next_command,
> -           _("Display next register group."),
> -           &tuireglist);
> -  add_cmd ("prev", class_tui, tui_reg_prev_command,
> -           _("Display previous register group."),
> -           &tuireglist);

(I notice we lose these expanded reggroup descriptions ("floating point"
vs "float").  We could save them by adding a "long_name" string
field to "struct reggroup", which doesn't look complicated.
Though I guess this isn't a big deal.  For another day.

(We could then reuse the reggroup description in "maint print reggroups".
Hmm, why is that a maintenance command, if we can use the reggroup
names in "info register"?  (and "help info registers" doesn't talk
about register groups, even though they are accepted.  Gah, lots of
small paper cuts.  :-/)
)

Thanks,
Pedro Alves