Re: [PATCH 03/14] Simplify tui_data_window::show_register_group

[Andrew Burgess <aburgess@redhat.com>]

Tom Tromey <tom@tromey.com> writes:

> This simplifies tui_data_window::show_register_group, renaming it as
> well.  The old method had two loops to iterate over all the registers
> for the arch, but in the new scheme, the vector is set up when
> switching groups, and then updates simply iterate over the vector.
>
> tui_data_item_window is made self-updating, which also clarifies the
> code somewhat.
> ---
>  gdb/tui/tui-regs.c | 104 ++++++++++++++++++++---------------------------------
>  gdb/tui/tui-regs.h |  16 ++++++---
>  2 files changed, 49 insertions(+), 71 deletions(-)
>
> diff --git a/gdb/tui/tui-regs.c b/gdb/tui/tui-regs.c
> index 91fbf75c250..7b8bf323f50 100644
> --- a/gdb/tui/tui-regs.c
> +++ b/gdb/tui/tui-regs.c
> @@ -104,21 +104,17 @@ tui_register_format (frame_info_ptr frame, int regnum)
>  /* Get the register value from the given frame and format it for the
>     display.  When changedp is set, check if the new register value has
>     changed with respect to the previous call.  */
> -static void
> -tui_get_register (frame_info_ptr frame,
> -		  struct tui_data_item_window *data, 
> -		  int regnum, bool *changedp)
> +void
> +tui_data_item_window::update (const frame_info_ptr &frame)

The comment on this function needs updating.

>  {
> -  if (changedp)
> -    *changedp = false;
>    if (target_has_registers ())
>      {
> -      std::string new_content = tui_register_format (frame, regnum);
> +      std::string new_content = tui_register_format (frame, regno);
>  
> -      if (changedp != NULL && data->content != new_content)
> -	*changedp = true;
> +      if (content != new_content)
> +	highlight = true;
>  
> -      data->content = std::move (new_content);
> +      content = std::move (new_content);
>      }
>  }
>  
> @@ -178,13 +174,11 @@ tui_data_window::show_registers (const reggroup *group)
>  
>    if (target_has_registers () && target_has_stack () && target_has_memory ())
>      {
> -      show_register_group (group, get_selected_frame (NULL),
> -			   group == m_current_group);
> +      update_register_data (group, get_selected_frame (NULL));

Maybe s/NULL/nullptr/ ?

>  
>        /* Clear all notation of changed values.  */
>        for (auto &&data_item_win : m_regs_content)
>  	data_item_win.highlight = false;
> -      m_current_group = group;
>      }
>    else
>      {
> @@ -201,63 +195,43 @@ tui_data_window::show_registers (const reggroup *group)
>     refresh_values_only is true.  */
>  
>  void
> -tui_data_window::show_register_group (const reggroup *group,
> -				      frame_info_ptr frame, 
> -				      bool refresh_values_only)
> +tui_data_window::update_register_data (const reggroup *group,
> +				       frame_info_ptr frame)

The comment on this function needs updating.

>  {
> -  struct gdbarch *gdbarch = get_frame_arch (frame);
> -  int nr_regs;
> -  int regnum, pos;
> -
> -  /* Make a new title showing which group we display.  */
> -  this->set_title (string_printf ("Register group: %s", group->name ()));
> -
> -  /* See how many registers must be displayed.  */
> -  nr_regs = 0;
> -  for (regnum = 0; regnum < gdbarch_num_cooked_regs (gdbarch); regnum++)
> +  if (group != m_current_group)
>      {
> -      const char *name;
> -
> -      /* Must be in the group.  */
> -      if (!gdbarch_register_reggroup_p (gdbarch, regnum, group))
> -	continue;
> -
> -      /* If the register name is empty, it is undefined for this
> -	 processor, so don't display anything.  */
> -      name = gdbarch_register_name (gdbarch, regnum);
> -      if (*name == '\0')
> -	continue;
> -
> -      nr_regs++;
> -    }
> +      m_current_group = group;
>  
> -  m_regs_content.resize (nr_regs);
> +      /* Make a new title showing which group we display.  */
> +      this->set_title (string_printf ("Register group: %s", group->name ()));
>  
> -  /* Now set the register names and values.  */
> -  pos = 0;
> -  for (regnum = 0; regnum < gdbarch_num_cooked_regs (gdbarch); regnum++)
> -    {
> -      struct tui_data_item_window *data_item_win;
> -      const char *name;
> +      /* Create the registers.  */
> +      m_regs_content.clear ();
>  
> -      /* Must be in the group.  */
> -      if (!gdbarch_register_reggroup_p (gdbarch, regnum, group))
> -	continue;
> +      struct gdbarch *gdbarch = get_frame_arch (frame);
> +      for (int regnum = 0;
> +	   regnum < gdbarch_num_cooked_regs (gdbarch);
> +	   regnum++)
> +	{
> +	  /* Must be in the group.  */
> +	  if (!gdbarch_register_reggroup_p (gdbarch, regnum, group))
> +	    continue;
>  
> -      /* If the register name is empty, it is undefined for this
> -	 processor, so don't display anything.  */
> -      name = gdbarch_register_name (gdbarch, regnum);
> -      if (*name == '\0')
> -	continue;
> +	  /* If the register name is empty, it is undefined for this
> +	     processor, so don't display anything.  */
> +	  const char *name = gdbarch_register_name (gdbarch, regnum);
> +	  if (*name == '\0')
> +	    continue;
>  
> -      data_item_win = &m_regs_content[pos];
> -      if (!refresh_values_only)
> -	{
> -	  data_item_win->regno = regnum;
> -	  data_item_win->highlight = false;
> +	  m_regs_content.emplace_back (regnum, frame);
>  	}
> -      tui_get_register (frame, data_item_win, regnum, 0);
> -      pos++;
> +    }
> +  else
> +    {
> +      /* The group did not change, so we can simply update each
> +	 item. */
> +      for (tui_data_item_window &reg : m_regs_content)
> +	reg.update (frame);
>      }
>  }
>  
> @@ -470,13 +444,11 @@ tui_data_window::check_register_values (frame_info_ptr frame)
>      show_registers (m_current_group);
>    else
>      {
> -      for (auto &&data_item_win : m_regs_content)
> +      for (tui_data_item_window &data_item_win : m_regs_content)
>  	{
>  	  bool was_hilighted = data_item_win.highlight;
>  
> -	  tui_get_register (frame, &data_item_win,
> -			    data_item_win.regno,
> -			    &data_item_win.highlight);
> +	  data_item_win.update (frame);
>  
>  	  /* Register windows whose y == 0 are outside the visible area.  */
>  	  if ((data_item_win.highlight || was_hilighted)
> diff --git a/gdb/tui/tui-regs.h b/gdb/tui/tui-regs.h
> index 1abd22cd382..9c451ba5996 100644
> --- a/gdb/tui/tui-regs.h
> +++ b/gdb/tui/tui-regs.h
> @@ -29,19 +29,26 @@
>  
>  struct tui_data_item_window
>  {
> -  tui_data_item_window () = default;
> +  tui_data_item_window (int regno, const frame_info_ptr &frame)
> +    : regno (regno)
> +  {
> +    update (frame);
> +    highlight = false;
> +  }
>  
>    DISABLE_COPY_AND_ASSIGN (tui_data_item_window);
>  
>    tui_data_item_window (tui_data_item_window &&) = default;
>  
> +  void update (const frame_info_ptr &frame);
> +
>    void rerender (WINDOW *handle, int field_width);
>  
>    /* Location.  */
>    int x = 0;
>    int y = 0;
>    /* The register number.  */
> -  int regno = -1;
> +  int regno;

If we're touching this anyway, maybe we could take the opportunity to
make this private?

Thanks,
Andrew

>    bool highlight = false;
>    bool visible = false;
>    std::string content;
> @@ -104,9 +111,8 @@ struct tui_data_window : public tui_win_info
>       display off the end of the register display.  */
>    void display_reg_element_at_line (int start_element_no, int start_line_no);
>  
> -  void show_register_group (const reggroup *group,
> -			    frame_info_ptr frame,
> -			    bool refresh_values_only);
> +  void update_register_data (const reggroup *group,
> +			     frame_info_ptr frame);
>  
>    /* Answer the number of the last line in the regs display.  If there
>       are no registers (-1) is returned.  */
>
> -- 
> 2.43.0