[03/14] Simplify tui_data_window::show_register_group

  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(-)
  

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
  

>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

>> +void
>> +tui_data_item_window::update (const frame_info_ptr &frame)

Andrew> The comment on this function needs updating.

>> +      update_register_data (group, get_selected_frame (NULL));

Andrew> Maybe s/NULL/nullptr/ ?

>> +tui_data_window::update_register_data (const reggroup *group,
>> +				       frame_info_ptr frame)

Andrew> The comment on this function needs updating.

>> /* The register number.  */
>> -  int regno = -1;
>> +  int regno;

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

I made these changes.

Tom