[PATCH 00/14] Cleanups for the TUi register window

[PATCH 00/14] Cleanups for the TUi register window

[Tom Tromey]

This series is a grab-bag of cleanups to the TUI register window.

I've tried to untangle the code somewhat, with the goal being removing
the extra rerender overload.

This series also fixes the 'exit' bug and removes the hacky and
unnecessary recursion flag from tui-hooks.c.

More cleanups here are possible -- the register window is pretty old
code and pretty ugly.  In particular:

* the layout code is still pretty bad

* it doesn't really make sense for check_register_values to accept a
  frame, because the window can only really ever use the selected
  frame anyway

* there's no horizontal scrolling, but for vector registers this might
  be nice (there's a bug about this)

---
Tom Tromey (14):
      Use pop_back in tui_register_format
      Minor C++ cleanups in tui-regs.c
      Simplify tui_data_window::show_register_group
      Rename tui_data_item_window -> tui_register_info
      Remove tui_register_info::visible
      Move scrollok call in register window
      Simplify update_register_data
      Remove the TUI register window rerender overload
      Simplify tui_data_win::erase_data_content
      Remove tui_refreshing_registers
      Remove redundant check from tui_refresh_frame_and_register_information
      Return void from tui_show_frame_info
      Rename show_registers -> set_register_group
      Update TUI register window when the inferior exits

 gdb/testsuite/gdb.tui/regs.exp |   8 ++
 gdb/tui/tui-hooks.c            |  36 +++----
 gdb/tui/tui-regs.c             | 220 +++++++++++++++--------------------------
 gdb/tui/tui-regs.h             |  47 +++++----
 gdb/tui/tui-stack.c            |  16 ++-
 gdb/tui/tui-stack.h            |   2 +-
 6 files changed, 137 insertions(+), 192 deletions(-)
---
base-commit: 2757c1c65fd6ba10c55ba5cf38d600814cf9dc1b
change-id: 20231217-tui-regs-cleanup-36d8f390a65a

Best regards,
-- 
Tom Tromey <tom@tromey.com>

[PATCH 01/14] Use pop_back in tui_register_format

[Tom Tromey]

tui_register_format can use string::pop_back now.
---
 gdb/tui/tui-regs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/tui/tui-regs.c b/gdb/tui/tui-regs.c
index 4ed3a2ee88e..6aabae23e69 100644
--- a/gdb/tui/tui-regs.c
+++ b/gdb/tui/tui-regs.c
@@ -96,7 +96,7 @@ tui_register_format (frame_info_ptr frame, int regnum)
   /* Remove the possible \n.  */
   std::string str = stream.release ();
   if (!str.empty () && str.back () == '\n')
-    str.resize (str.size () - 1);
+    str.pop_back ();
 
   return str;
 }

-- 
2.43.0

[PATCH 02/14] Minor C++ cleanups in tui-regs.c

[Tom Tromey]

This changes a couple of spots to use nullptr rather than 0, and
changes an int to a bool.
---
 gdb/tui/tui-regs.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/gdb/tui/tui-regs.c b/gdb/tui/tui-regs.c
index 6aabae23e69..91fbf75c250 100644
--- a/gdb/tui/tui-regs.c
+++ b/gdb/tui/tui-regs.c
@@ -173,7 +173,7 @@ tui_data_window::first_reg_element_no_inline (int line_no) const
 void
 tui_data_window::show_registers (const reggroup *group)
 {
-  if (group == 0)
+  if (group == nullptr)
     group = general_reggroup;
 
   if (target_has_registers () && target_has_stack () && target_has_memory ())
@@ -188,7 +188,7 @@ tui_data_window::show_registers (const reggroup *group)
     }
   else
     {
-      m_current_group = 0;
+      m_current_group = nullptr;
       m_regs_content.clear ();
     }
 
@@ -472,9 +472,7 @@ tui_data_window::check_register_values (frame_info_ptr frame)
     {
       for (auto &&data_item_win : m_regs_content)
 	{
-	  int was_hilighted;
-
-	  was_hilighted = data_item_win.highlight;
+	  bool was_hilighted = data_item_win.highlight;
 
 	  tui_get_register (frame, &data_item_win,
 			    data_item_win.regno,

-- 
2.43.0

[PATCH 03/14] Simplify tui_data_window::show_register_group

[Tom Tromey]

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)
 {
-  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));
 
       /* 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)
 {
-  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;
   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

[PATCH 04/14] Rename tui_data_item_window -> tui_register_info

[Tom Tromey]

tui_data_item_window used to hold a curses window, but we removed that
ages ago.  Now it just holds information about a single register.
This patch renames the class to make it more clearly reflect its
meaning.
---
 gdb/tui/tui-regs.c | 13 +++++--------
 gdb/tui/tui-regs.h | 14 +++++++-------
 2 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/gdb/tui/tui-regs.c b/gdb/tui/tui-regs.c
index 7b8bf323f50..f87d1ff8721 100644
--- a/gdb/tui/tui-regs.c
+++ b/gdb/tui/tui-regs.c
@@ -105,15 +105,12 @@ tui_register_format (frame_info_ptr frame, int regnum)
    display.  When changedp is set, check if the new register value has
    changed with respect to the previous call.  */
 void
-tui_data_item_window::update (const frame_info_ptr &frame)
+tui_register_info::update (const frame_info_ptr &frame)
 {
   if (target_has_registers ())
     {
       std::string new_content = tui_register_format (frame, regno);
-
-      if (content != new_content)
-	highlight = true;
-
+      highlight = content != new_content;
       content = std::move (new_content);
     }
 }
@@ -230,7 +227,7 @@ tui_data_window::update_register_data (const reggroup *group,
     {
       /* The group did not change, so we can simply update each
 	 item. */
-      for (tui_data_item_window &reg : m_regs_content)
+      for (tui_register_info &reg : m_regs_content)
 	reg.update (frame);
     }
 }
@@ -444,7 +441,7 @@ tui_data_window::check_register_values (frame_info_ptr frame)
     show_registers (m_current_group);
   else
     {
-      for (tui_data_item_window &data_item_win : m_regs_content)
+      for (tui_register_info &data_item_win : m_regs_content)
 	{
 	  bool was_hilighted = data_item_win.highlight;
 
@@ -463,7 +460,7 @@ tui_data_window::check_register_values (frame_info_ptr frame)
 /* Display a register in a window.  If hilite is TRUE, then the value
    will be displayed in reverse video.  */
 void
-tui_data_item_window::rerender (WINDOW *handle, int field_width)
+tui_register_info::rerender (WINDOW *handle, int field_width)
 {
   /* In case the regs window is not boxed, we'll write the last char in the
      last line here, causing a scroll, so prevent that.  */
diff --git a/gdb/tui/tui-regs.h b/gdb/tui/tui-regs.h
index 9c451ba5996..1a9f30fa270 100644
--- a/gdb/tui/tui-regs.h
+++ b/gdb/tui/tui-regs.h
@@ -25,20 +25,20 @@
 #include "tui/tui-data.h"
 #include "reggroups.h"
 
-/* A data item window.  */
+/* Information about the display of a single register.  */
 
-struct tui_data_item_window
+struct tui_register_info
 {
-  tui_data_item_window (int regno, const frame_info_ptr &frame)
+  tui_register_info (int regno, const frame_info_ptr &frame)
     : regno (regno)
   {
     update (frame);
     highlight = false;
   }
 
-  DISABLE_COPY_AND_ASSIGN (tui_data_item_window);
+  DISABLE_COPY_AND_ASSIGN (tui_register_info);
 
-  tui_data_item_window (tui_data_item_window &&) = default;
+  tui_register_info (tui_register_info &&) = default;
 
   void update (const frame_info_ptr &frame);
 
@@ -133,8 +133,8 @@ struct tui_data_window : public tui_win_info
 
   void erase_data_content (const char *prompt);
 
-  /* Windows that are used to display registers.  */
-  std::vector<tui_data_item_window> m_regs_content;
+  /* Information about each register in the current register group.  */
+  std::vector<tui_register_info> m_regs_content;
   int m_regs_column_count = 0;
   const reggroup *m_current_group = nullptr;
 

-- 
2.43.0

[PATCH 05/14] Remove tui_register_info::visible

[Tom Tromey]

tui_register_info::visible is redundant with the fact that y==0 means
that the register is not visible -- and some spots already check the
latter.  This patch removes this member in favor of having a single
indication of the register's visibility.  This change makes it clear
that delete_data_content_windows is not needed, so this is removed as
well.
---
 gdb/tui/tui-regs.c | 15 +--------------
 gdb/tui/tui-regs.h |  1 -
 2 files changed, 1 insertion(+), 15 deletions(-)

diff --git a/gdb/tui/tui-regs.c b/gdb/tui/tui-regs.c
index f87d1ff8721..9108e460d6f 100644
--- a/gdb/tui/tui-regs.c
+++ b/gdb/tui/tui-regs.c
@@ -269,7 +269,6 @@ tui_data_window::display_registers_from (int start_element_no)
 	  /* Create the window if necessary.  */
 	  m_regs_content[i].x = box_width () + (m_item_width * j);
 	  m_regs_content[i].y = cur_y;
-	  m_regs_content[i].visible = true;
 	  m_regs_content[i].rerender (handle.get (), m_item_width);
 	  i++;		/* Next register.  */
 	}
@@ -347,23 +346,13 @@ tui_data_window::first_data_item_displayed ()
 {
   for (int i = 0; i < m_regs_content.size (); i++)
     {
-      if (m_regs_content[i].visible)
+      if (m_regs_content[i].y > 0)
 	return i;
     }
 
   return -1;
 }
 
-/* See tui-regs.h.  */
-
-void
-tui_data_window::delete_data_content_windows ()
-{
-  for (auto &win : m_regs_content)
-    win.visible = false;
-}
-
-
 void
 tui_data_window::erase_data_content (const char *prompt)
 {
@@ -401,7 +390,6 @@ tui_data_window::rerender (bool toplevel)
   else
     {
       erase_data_content (NULL);
-      delete_data_content_windows ();
       display_registers_from (0);
     }
 }
@@ -426,7 +414,6 @@ tui_data_window::do_scroll_vertical (int num_to_scroll)
     {
       first_line += num_to_scroll;
       erase_data_content (NULL);
-      delete_data_content_windows ();
       display_registers_from_line (first_line);
     }
 }
diff --git a/gdb/tui/tui-regs.h b/gdb/tui/tui-regs.h
index 1a9f30fa270..701b8707947 100644
--- a/gdb/tui/tui-regs.h
+++ b/gdb/tui/tui-regs.h
@@ -50,7 +50,6 @@ struct tui_register_info
   /* The register number.  */
   int regno;
   bool highlight = false;
-  bool visible = false;
   std::string content;
 };
 

-- 
2.43.0

[PATCH 06/14] Move scrollok call in register window

[Tom Tromey]

The register window calls scrollok each time a register is written to
the window.  However, we only need to call this once, at the start of
display.  (We could actually call it just once when the window is
made, but that would involve making another method virtual or adding a
new member -- both which I think are worse than this approach.)
---
 gdb/tui/tui-regs.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/gdb/tui/tui-regs.c b/gdb/tui/tui-regs.c
index 9108e460d6f..0ad23e93778 100644
--- a/gdb/tui/tui-regs.c
+++ b/gdb/tui/tui-regs.c
@@ -237,6 +237,10 @@ tui_data_window::update_register_data (const reggroup *group,
 void
 tui_data_window::display_registers_from (int start_element_no)
 {
+  /* In case the regs window is not boxed, we'll write the last char in the
+     last line here, causing a scroll, so prevent that.  */
+  scrollok (handle.get (), FALSE);
+
   int max_len = 0;
   for (auto &&data_item_win : m_regs_content)
     {
@@ -449,10 +453,6 @@ tui_data_window::check_register_values (frame_info_ptr frame)
 void
 tui_register_info::rerender (WINDOW *handle, int field_width)
 {
-  /* In case the regs window is not boxed, we'll write the last char in the
-     last line here, causing a scroll, so prevent that.  */
-  scrollok (handle, FALSE);
-
   if (highlight)
     /* We ignore the return value, casting it to void in order to avoid
        a compiler warning.  The warning itself was introduced by a patch

-- 
2.43.0

[PATCH 07/14] Simplify update_register_data

[Tom Tromey]

This changes update_register_data to always update the register data.
The idea here is that this is really only called when either the
desired register group changes, or when gdb transitions from not
having a frame to having a frame.

show_registers is also simplified slightly to account for this.
---
 gdb/tui/tui-regs.c | 57 ++++++++++++++++++++----------------------------------
 1 file changed, 21 insertions(+), 36 deletions(-)

diff --git a/gdb/tui/tui-regs.c b/gdb/tui/tui-regs.c
index 0ad23e93778..7b6b669fe51 100644
--- a/gdb/tui/tui-regs.c
+++ b/gdb/tui/tui-regs.c
@@ -170,15 +170,10 @@ tui_data_window::show_registers (const reggroup *group)
     group = general_reggroup;
 
   if (target_has_registers () && target_has_stack () && target_has_memory ())
-    {
-      update_register_data (group, get_selected_frame (NULL));
-
-      /* Clear all notation of changed values.  */
-      for (auto &&data_item_win : m_regs_content)
-	data_item_win.highlight = false;
-    }
+    update_register_data (group, get_selected_frame (nullptr));
   else
     {
+      set_title (_("Registers"));
       m_current_group = nullptr;
       m_regs_content.clear ();
     }
@@ -195,40 +190,30 @@ void
 tui_data_window::update_register_data (const reggroup *group,
 				       frame_info_ptr frame)
 {
-  if (group != m_current_group)
-    {
-      m_current_group = group;
+  m_current_group = group;
 
-      /* Make a new title showing which group we display.  */
-      this->set_title (string_printf ("Register group: %s", group->name ()));
+  /* Make a new title showing which group we display.  */
+  this->set_title (string_printf ("Register group: %s", group->name ()));
 
-      /* Create the registers.  */
-      m_regs_content.clear ();
+  /* Create the registers.  */
+  m_regs_content.clear ();
 
-      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;
+  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.  */
-	  const char *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;
 
-	  m_regs_content.emplace_back (regnum, frame);
-	}
-    }
-  else
-    {
-      /* The group did not change, so we can simply update each
-	 item. */
-      for (tui_register_info &reg : m_regs_content)
-	reg.update (frame);
+      m_regs_content.emplace_back (regnum, frame);
     }
 }
 

-- 
2.43.0

[PATCH 08/14] Remove the TUI register window rerender overload

[Tom Tromey]

After these restructurings, it should be clear that the rerender
overload can be removed from the TUI register window.  This is done by
moving a bit more logic from show_registers into update_register_data.
After this, update_register_data simply updates the internal state,
and rerender will write to the screen.  All the actual rendering work
is done, ultimately, by display_registers_from.  This split between
updating the mode and rendering makes it clear that the recursive case
can't happen any longer.
---
 gdb/tui/tui-regs.c | 55 ++++++++++++++++++++++++------------------------------
 gdb/tui/tui-regs.h | 14 ++++++--------
 2 files changed, 30 insertions(+), 39 deletions(-)

diff --git a/gdb/tui/tui-regs.c b/gdb/tui/tui-regs.c
index 7b6b669fe51..5726dc3fea8 100644
--- a/gdb/tui/tui-regs.c
+++ b/gdb/tui/tui-regs.c
@@ -165,31 +165,33 @@ tui_data_window::first_reg_element_no_inline (int line_no) const
    and refresh the window.  */
 void
 tui_data_window::show_registers (const reggroup *group)
+{
+  update_register_data (group);
+  rerender ();
+}
+
+/* Set the data window to display the registers of the register group
+   using the given frame.  Values are refreshed only when
+   refresh_values_only is true.  */
+
+void
+tui_data_window::update_register_data (const reggroup *group)
 {
   if (group == nullptr)
     group = general_reggroup;
 
-  if (target_has_registers () && target_has_stack () && target_has_memory ())
-    update_register_data (group, get_selected_frame (nullptr));
-  else
+  if (!target_has_registers ()
+      || !target_has_stack ()
+      || !target_has_memory ())
     {
-      set_title (_("Registers"));
+      set_title ("");
       m_current_group = nullptr;
       m_regs_content.clear ();
+      return;
     }
 
-  rerender (false);
-}
-
-
-/* Set the data window to display the registers of the register group
-   using the given frame.  Values are refreshed only when
-   refresh_values_only is true.  */
+  frame_info_ptr frame = get_selected_frame (nullptr);
 
-void
-tui_data_window::update_register_data (const reggroup *group,
-				       frame_info_ptr frame)
-{
   m_current_group = group;
 
   /* Make a new title showing which group we display.  */
@@ -222,6 +224,9 @@ tui_data_window::update_register_data (const reggroup *group,
 void
 tui_data_window::display_registers_from (int start_element_no)
 {
+  werase (handle.get ());
+  check_and_display_highlight_if_needed ();
+
   /* In case the regs window is not boxed, we'll write the last char in the
      last line here, causing a scroll, so prevent that.  */
   scrollok (handle.get (), FALSE);
@@ -358,29 +363,18 @@ tui_data_window::erase_data_content (const char *prompt)
 	x_pos = half_width - strlen (prompt);
       display_string (height / 2, x_pos, prompt);
     }
-  tui_wrefresh (handle.get ());
 }
 
 /* See tui-regs.h.  */
 
 void
-tui_data_window::rerender (bool toplevel)
+tui_data_window::rerender ()
 {
   if (m_regs_content.empty ())
-    {
-      if (toplevel && has_stack_frames ())
-	{
-	  frame_info_ptr fi = get_selected_frame (NULL);
-	  check_register_values (fi);
-	}
-      else
-	erase_data_content (_("[ Register Values Unavailable ]"));
-    }
+    erase_data_content (_("[ Register Values Unavailable ]"));
   else
-    {
-      erase_data_content (NULL);
-      display_registers_from (0);
-    }
+    display_registers_from (0);
+  tui_wrefresh (handle.get ());
 }
 
 
@@ -402,7 +396,6 @@ tui_data_window::do_scroll_vertical (int num_to_scroll)
   if (first_line >= 0)
     {
       first_line += num_to_scroll;
-      erase_data_content (NULL);
       display_registers_from_line (first_line);
     }
 }
diff --git a/gdb/tui/tui-regs.h b/gdb/tui/tui-regs.h
index 701b8707947..b31a5a3e5b8 100644
--- a/gdb/tui/tui-regs.h
+++ b/gdb/tui/tui-regs.h
@@ -56,7 +56,10 @@ struct tui_register_info
 /* The TUI registers window.  */
 struct tui_data_window : public tui_win_info
 {
-  tui_data_window () = default;
+  tui_data_window ()
+  {
+    update_register_data (nullptr);
+  }
 
   DISABLE_COPY_AND_ASSIGN (tui_data_window);
 
@@ -81,11 +84,7 @@ struct tui_data_window : public tui_win_info
   {
   }
 
-  void rerender (bool toplevel);
-  void rerender () override
-  {
-    rerender (true);
-  }
+  void rerender () override;
 
 private:
 
@@ -110,8 +109,7 @@ 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 update_register_data (const reggroup *group,
-			     frame_info_ptr frame);
+  void update_register_data (const reggroup *group);
 
   /* Answer the number of the last line in the regs display.  If there
      are no registers (-1) is returned.  */

-- 
2.43.0

[PATCH 09/14] Simplify tui_data_win::erase_data_content

[Tom Tromey]

There's only a single call to tui_data_win::erase_data_content now, so
remove the parameter and make it just render the "empty window" text.
---
 gdb/tui/tui-regs.c | 22 ++++++++++------------
 gdb/tui/tui-regs.h |  2 +-
 2 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/gdb/tui/tui-regs.c b/gdb/tui/tui-regs.c
index 5726dc3fea8..bf752c267ff 100644
--- a/gdb/tui/tui-regs.c
+++ b/gdb/tui/tui-regs.c
@@ -348,21 +348,19 @@ tui_data_window::first_data_item_displayed ()
 }
 
 void
-tui_data_window::erase_data_content (const char *prompt)
+tui_data_window::erase_data_content ()
 {
   werase (handle.get ());
   check_and_display_highlight_if_needed ();
-  if (prompt != NULL)
-    {
-      int half_width = (width - box_size ()) / 2;
-      int x_pos;
 
-      if (strlen (prompt) >= half_width)
-	x_pos = 1;
-      else
-	x_pos = half_width - strlen (prompt);
-      display_string (height / 2, x_pos, prompt);
-    }
+  const char *prompt = _("[ Register Values Unavailable ]");
+  int half_width = (width - box_size ()) / 2;
+  int x_pos;
+  if (strlen (prompt) >= half_width)
+    x_pos = 1;
+  else
+    x_pos = half_width - strlen (prompt);
+  display_string (height / 2, x_pos, prompt);
 }
 
 /* See tui-regs.h.  */
@@ -371,7 +369,7 @@ void
 tui_data_window::rerender ()
 {
   if (m_regs_content.empty ())
-    erase_data_content (_("[ Register Values Unavailable ]"));
+    erase_data_content ();
   else
     display_registers_from (0);
   tui_wrefresh (handle.get ());
diff --git a/gdb/tui/tui-regs.h b/gdb/tui/tui-regs.h
index b31a5a3e5b8..29518a6f714 100644
--- a/gdb/tui/tui-regs.h
+++ b/gdb/tui/tui-regs.h
@@ -128,7 +128,7 @@ struct tui_data_window : public tui_win_info
      done when the data window is scrolled.  */
   void delete_data_content_windows ();
 
-  void erase_data_content (const char *prompt);
+  void erase_data_content ();
 
   /* Information about each register in the current register group.  */
   std::vector<tui_register_info> m_regs_content;

-- 
2.43.0

[PATCH 10/14] Remove tui_refreshing_registers

[Tom Tromey]

The comment by tui_refreshing_registers mentions a hook that no longer
exists.  However, maybe the comment is wrong.

The code paths touching tui_refreshing_registers can only be called in two places:

1. From the before_prompt observer.  This is only called when a prompt
   is about to be displayed.

2. From the register_changed observer.  This is only called when
   value_assign changes a register value.

From this it seems clear that the recursion case here cannot in fact
occur.  This patch removes the variable.
---
 gdb/tui/tui-hooks.c | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/gdb/tui/tui-hooks.c b/gdb/tui/tui-hooks.c
index 6525f0f2b6c..e47607fefa9 100644
--- a/gdb/tui/tui-hooks.c
+++ b/gdb/tui/tui-hooks.c
@@ -63,9 +63,6 @@ static void
 tui_all_objfiles_removed (program_space *pspace)
 { tui_on_objfiles_changed (); }
 
-/* Prevent recursion of deprecated_register_changed_hook().  */
-static bool tui_refreshing_registers = false;
-
 /* Observer for the register_changed notification.  */
 
 static void
@@ -82,12 +79,7 @@ tui_register_changed (frame_info_ptr frame, int regno)
      up in the other.  So we always use the selected frame here, and ignore
      FRAME.  */
   fi = get_selected_frame (NULL);
-  if (!tui_refreshing_registers)
-    {
-      tui_refreshing_registers = true;
-      TUI_DATA_WIN->check_register_values (fi);
-      tui_refreshing_registers = false;
-    }
+  TUI_DATA_WIN->check_register_values (fi);
 }
 
 /* Breakpoint creation hook.
@@ -145,11 +137,7 @@ tui_refresh_frame_and_register_information ()
       /* Refresh the register window if it's visible.  */
       if (tui_is_window_visible (DATA_WIN)
 	  && (frame_info_changed_p || from_stack))
-	{
-	  tui_refreshing_registers = true;
-	  TUI_DATA_WIN->check_register_values (fi);
-	  tui_refreshing_registers = false;
-	}
+	TUI_DATA_WIN->check_register_values (fi);
     }
   else if (!from_stack)
     {

-- 
2.43.0

[PATCH 11/14] Remove redundant check from tui_refresh_frame_and_register_information

[Tom Tromey]

tui_refresh_frame_and_register_information checks 'from_stack' in a
block that's already guarded by a 'from_stack' check.  This patch
removes the redundant check.
---
 gdb/tui/tui-hooks.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/gdb/tui/tui-hooks.c b/gdb/tui/tui-hooks.c
index e47607fefa9..f1e4978a40a 100644
--- a/gdb/tui/tui-hooks.c
+++ b/gdb/tui/tui-hooks.c
@@ -132,11 +132,10 @@ tui_refresh_frame_and_register_information ()
 
       /* Display the frame position (even if there is no symbols or
 	 the PC is not known).  */
-      bool frame_info_changed_p = tui_show_frame_info (fi);
+      tui_show_frame_info (fi);
 
       /* Refresh the register window if it's visible.  */
-      if (tui_is_window_visible (DATA_WIN)
-	  && (frame_info_changed_p || from_stack))
+      if (tui_is_window_visible (DATA_WIN))
 	TUI_DATA_WIN->check_register_values (fi);
     }
   else if (!from_stack)

-- 
2.43.0

[PATCH 12/14] Return void from tui_show_frame_info

[Tom Tromey]

Nothing uses the tui_show_frame_info result any more, so change it to
return void.
---
 gdb/tui/tui-stack.c | 16 ++++++----------
 gdb/tui/tui-stack.h |  2 +-
 2 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/gdb/tui/tui-stack.c b/gdb/tui/tui-stack.c
index 8bf65ea3556..f40948b2305 100644
--- a/gdb/tui/tui-stack.c
+++ b/gdb/tui/tui-stack.c
@@ -262,13 +262,11 @@ tui_locator_window::rerender ()
   wmove (handle.get (), 0, 0);
 }
 
-/* Function to print the frame information for the TUI.  The windows are
-   refreshed only if frame information has changed since the last refresh.
+/* Function to print the frame information for the TUI.  The windows
+   are refreshed only if frame information has changed since the last
+   refresh.  */
 
-   Return true if frame information has changed (and windows
-   subsequently refreshed), false otherwise.  */
-
-bool
+void
 tui_show_frame_info (frame_info_ptr fi)
 {
   bool locator_changed_p;
@@ -292,7 +290,7 @@ tui_show_frame_info (frame_info_ptr fi)
 	 not changed.  If frame information has not changed, then the windows'
 	 contents will not change.  So don't bother refreshing the windows.  */
       if (!locator_changed_p)
-	return false;
+	return;
 
       for (struct tui_source_window_base *win_info : tui_source_windows ())
 	{
@@ -307,13 +305,11 @@ tui_show_frame_info (frame_info_ptr fi)
       locator_changed_p = tui_location.set_location (NULL, sal, "");
 
       if (!locator_changed_p)
-	return false;
+	return;
 
       for (struct tui_source_window_base *win_info : tui_source_windows ())
 	win_info->erase_source_content ();
     }
-
-  return true;
 }
 
 void
diff --git a/gdb/tui/tui-stack.h b/gdb/tui/tui-stack.h
index ca95b2bf78a..3f71a4165de 100644
--- a/gdb/tui/tui-stack.h
+++ b/gdb/tui/tui-stack.h
@@ -51,6 +51,6 @@ struct tui_locator_window
 };
 
 extern void tui_show_locator_content (void);
-extern bool tui_show_frame_info (frame_info_ptr);
+extern void tui_show_frame_info (frame_info_ptr);
 
 #endif /* TUI_TUI_STACK_H */

-- 
2.43.0

[PATCH 13/14] Rename show_registers -> set_register_group

[Tom Tromey]

This renames a method on the TUI register window to reflect its real
purpose.
---
 gdb/tui/tui-regs.c | 6 +++---
 gdb/tui/tui-regs.h | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/gdb/tui/tui-regs.c b/gdb/tui/tui-regs.c
index bf752c267ff..248e6395cd7 100644
--- a/gdb/tui/tui-regs.c
+++ b/gdb/tui/tui-regs.c
@@ -164,7 +164,7 @@ tui_data_window::first_reg_element_no_inline (int line_no) const
 /* Show the registers of the given group in the data window
    and refresh the window.  */
 void
-tui_data_window::show_registers (const reggroup *group)
+tui_data_window::set_register_group (const reggroup *group)
 {
   update_register_data (group);
   rerender ();
@@ -405,7 +405,7 @@ void
 tui_data_window::check_register_values (frame_info_ptr frame)
 {
   if (m_regs_content.empty ())
-    show_registers (m_current_group);
+    set_register_group (m_current_group);
   else
     {
       for (tui_register_info &data_item_win : m_regs_content)
@@ -543,7 +543,7 @@ tui_reg_command (const char *args, int from_tty)
       if (match == NULL)
 	error (_("unknown register group '%s'"), args);
 
-      TUI_DATA_WIN->show_registers (match);
+      TUI_DATA_WIN->set_register_group (match);
     }
   else
     {
diff --git a/gdb/tui/tui-regs.h b/gdb/tui/tui-regs.h
index 29518a6f714..a5cd30b7160 100644
--- a/gdb/tui/tui-regs.h
+++ b/gdb/tui/tui-regs.h
@@ -70,7 +70,7 @@ struct tui_data_window : public tui_win_info
 
   void check_register_values (frame_info_ptr frame);
 
-  void show_registers (const reggroup *group);
+  void set_register_group (const reggroup *group);
 
   const reggroup *get_current_group () const
   {

-- 
2.43.0

[PATCH 14/14] Update TUI register window when the inferior exits

[Tom Tromey]

When the inferior exits, the TUI register window should clear.

Fixing this was mostly a matter of sticking an assignment into
tui_inferior_exit.  However, some changes to the register window
itself were also needed.

While working on this, I realized that the TUI register window would
not work correctly when moving between frames of different
architectures.  This patch attempts to fix this as well, though I have
no way to test it.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28600
---
 gdb/testsuite/gdb.tui/regs.exp |  8 +++++++
 gdb/tui/tui-hooks.c            | 17 +++++++++------
 gdb/tui/tui-regs.c             | 48 ++++++++++++++++++++++++++----------------
 gdb/tui/tui-regs.h             |  4 ++++
 4 files changed, 53 insertions(+), 24 deletions(-)

diff --git a/gdb/testsuite/gdb.tui/regs.exp b/gdb/testsuite/gdb.tui/regs.exp
index 0be99625b9f..c9e7cac72b7 100644
--- a/gdb/testsuite/gdb.tui/regs.exp
+++ b/gdb/testsuite/gdb.tui/regs.exp
@@ -30,6 +30,9 @@ if {![runto_main]} {
     return
 }
 
+# This is convenient later on.
+gdb_test_no_output "set confirm off"
+
 if {![Term::enter_tui]} {
     unsupported "TUI not supported"
     return
@@ -48,6 +51,11 @@ gdb_assert \
     { ![Term::check_region_contents_p 0 0 80 8 $re_reg_vals_unavailable] } \
     "Register values available"
 
+Term::command "kill"
+gdb_assert \
+    { [Term::check_region_contents_p 0 0 80 8 $re_reg_vals_unavailable] } \
+    "Register values no longer available"
+
 # Check that we can successfully cause the register window to appear
 # using the 'tui reg next' and 'tui reg prev' commands.
 foreach_with_prefix cmd { next prev } {
diff --git a/gdb/tui/tui-hooks.c b/gdb/tui/tui-hooks.c
index f1e4978a40a..3b5dd527fac 100644
--- a/gdb/tui/tui-hooks.c
+++ b/gdb/tui/tui-hooks.c
@@ -126,19 +126,23 @@ tui_refresh_frame_and_register_information ()
   target_terminal::scoped_restore_terminal_state term_state;
   target_terminal::ours_for_output ();
 
-  if (from_stack && has_stack_frames ())
+  if (from_stack)
     {
-      frame_info_ptr fi = get_selected_frame (NULL);
+      frame_info_ptr fi;
+      if (has_stack_frames ())
+	{
+	  fi = get_selected_frame (NULL);
 
-      /* Display the frame position (even if there is no symbols or
-	 the PC is not known).  */
-      tui_show_frame_info (fi);
+	  /* Display the frame position (even if there is no symbols or
+	     the PC is not known).  */
+	  tui_show_frame_info (fi);
+	}
 
       /* Refresh the register window if it's visible.  */
       if (tui_is_window_visible (DATA_WIN))
 	TUI_DATA_WIN->check_register_values (fi);
     }
-  else if (!from_stack)
+  else
     {
       /* Make sure that the source window is displayed.  */
       tui_add_win_to_layout (SRC_WIN);
@@ -169,6 +173,7 @@ tui_inferior_exit (struct inferior *inf)
   tui_set_key_mode (TUI_COMMAND_MODE);
   tui_show_frame_info (0);
   tui_display_main ();
+  from_stack = true;
 }
 
 /* Observer for the before_prompt notification.  */
diff --git a/gdb/tui/tui-regs.c b/gdb/tui/tui-regs.c
index 248e6395cd7..feaf3b5abb8 100644
--- a/gdb/tui/tui-regs.c
+++ b/gdb/tui/tui-regs.c
@@ -107,12 +107,9 @@ tui_register_format (frame_info_ptr frame, int regnum)
 void
 tui_register_info::update (const frame_info_ptr &frame)
 {
-  if (target_has_registers ())
-    {
-      std::string new_content = tui_register_format (frame, regno);
-      highlight = content != new_content;
-      content = std::move (new_content);
-    }
+  std::string new_content = tui_register_format (frame, regno);
+  highlight = content != new_content;
+  content = std::move (new_content);
 }
 
 /* See tui-regs.h.  */
@@ -186,13 +183,22 @@ tui_data_window::update_register_data (const reggroup *group)
     {
       set_title ("");
       m_current_group = nullptr;
+      m_gdbarch = nullptr;
       m_regs_content.clear ();
       return;
     }
 
   frame_info_ptr frame = get_selected_frame (nullptr);
+  struct gdbarch *gdbarch = get_frame_arch (frame);
+
+  if (m_current_group == group && m_gdbarch == gdbarch)
+    {
+      /* Nothing to do here.  */
+      return;
+    }
 
   m_current_group = group;
+  m_gdbarch = gdbarch;
 
   /* Make a new title showing which group we display.  */
   this->set_title (string_printf ("Register group: %s", group->name ()));
@@ -200,7 +206,6 @@ tui_data_window::update_register_data (const reggroup *group)
   /* Create the registers.  */
   m_regs_content.clear ();
 
-  struct gdbarch *gdbarch = get_frame_arch (frame);
   for (int regnum = 0;
        regnum < gdbarch_num_cooked_regs (gdbarch);
        regnum++)
@@ -404,24 +409,31 @@ tui_data_window::do_scroll_vertical (int num_to_scroll)
 void
 tui_data_window::check_register_values (frame_info_ptr frame)
 {
-  if (m_regs_content.empty ())
-    set_register_group (m_current_group);
+  if (frame == nullptr)
+    set_register_group (nullptr);
   else
     {
-      for (tui_register_info &data_item_win : m_regs_content)
+      /* If the frame architecture changed, we need to reset the
+	 register group.  */
+      struct gdbarch *gdbarch = get_frame_arch (frame);
+      if (gdbarch != m_gdbarch)
+	set_register_group (nullptr);
+      else
 	{
-	  bool was_hilighted = data_item_win.highlight;
+	  for (tui_register_info &data_item_win : m_regs_content)
+	    {
+	      bool was_hilighted = data_item_win.highlight;
 
-	  data_item_win.update (frame);
+	      data_item_win.update (frame);
 
-	  /* Register windows whose y == 0 are outside the visible area.  */
-	  if ((data_item_win.highlight || was_hilighted)
-	      && data_item_win.y > 0)
-	    data_item_win.rerender (handle.get (), m_item_width);
+	      /* Register windows whose y == 0 are outside the visible area.  */
+	      if ((data_item_win.highlight || was_hilighted)
+		  && data_item_win.y > 0)
+		data_item_win.rerender (handle.get (), m_item_width);
+	    }
 	}
+      tui_wrefresh (handle.get ());
     }
-
-  tui_wrefresh (handle.get ());
 }
 
 /* Display a register in a window.  If hilite is TRUE, then the value
diff --git a/gdb/tui/tui-regs.h b/gdb/tui/tui-regs.h
index a5cd30b7160..6c62938259f 100644
--- a/gdb/tui/tui-regs.h
+++ b/gdb/tui/tui-regs.h
@@ -137,6 +137,10 @@ struct tui_data_window : public tui_win_info
 
   /* Width of each register's display area.  */
   int m_item_width = 0;
+
+  /* Architecture of frame whose registers are being displayed, or
+     nullptr if the display is empty (i.e., there is no frame).  */
+  gdbarch *m_gdbarch = nullptr;
 };
 
 #endif /* TUI_TUI_REGS_H */

-- 
2.43.0

Re: [PATCH 00/14] Cleanups for the TUi register window

[Tom de Vries]

On 12/17/23 20:50, Tom Tromey wrote:
> This series is a grab-bag of cleanups to the TUI register window.
> 
> I've tried to untangle the code somewhat, with the goal being removing
> the extra rerender overload.
> 
> This series also fixes the 'exit' bug and removes the hacky and
> unnecessary recursion flag from tui-hooks.c.
> 

I've applied the series and tested the TUI test-cases, all looks good.

I've also confirmed that killing an inferior gets us "no registers 
available".

Tested-By: Tom de Vries <tdevries@suse.de>

Thanks,
- Tom

> More cleanups here are possible -- the register window is pretty old
> code and pretty ugly.  In particular:
> 
> * the layout code is still pretty bad
> 
> * it doesn't really make sense for check_register_values to accept a
>    frame, because the window can only really ever use the selected
>    frame anyway
> 
> * there's no horizontal scrolling, but for vector registers this might
>    be nice (there's a bug about this)
> 
> ---
> Tom Tromey (14):
>        Use pop_back in tui_register_format
>        Minor C++ cleanups in tui-regs.c
>        Simplify tui_data_window::show_register_group
>        Rename tui_data_item_window -> tui_register_info
>        Remove tui_register_info::visible
>        Move scrollok call in register window
>        Simplify update_register_data
>        Remove the TUI register window rerender overload
>        Simplify tui_data_win::erase_data_content
>        Remove tui_refreshing_registers
>        Remove redundant check from tui_refresh_frame_and_register_information
>        Return void from tui_show_frame_info
>        Rename show_registers -> set_register_group
>        Update TUI register window when the inferior exits
> 
>   gdb/testsuite/gdb.tui/regs.exp |   8 ++
>   gdb/tui/tui-hooks.c            |  36 +++----
>   gdb/tui/tui-regs.c             | 220 +++++++++++++++--------------------------
>   gdb/tui/tui-regs.h             |  47 +++++----
>   gdb/tui/tui-stack.c            |  16 ++-
>   gdb/tui/tui-stack.h            |   2 +-
>   6 files changed, 137 insertions(+), 192 deletions(-)
> ---
> base-commit: 2757c1c65fd6ba10c55ba5cf38d600814cf9dc1b
> change-id: 20231217-tui-regs-cleanup-36d8f390a65a
> 
> Best regards,

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

[Andrew Burgess]

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

Re: [PATCH 05/14] Remove tui_register_info::visible

[Hannes Domani]

 Am Sonntag, 17. Dezember 2023, 20:51:14 MEZ hat Tom Tromey <tom@tromey.com> Folgendes geschrieben:

> tui_register_info::visible is redundant with the fact that y==0 means
> that the register is not visible -- and some spots already check the
> latter.  This patch removes this member in favor of having a single
> indication of the register's visibility.  This change makes it clear
> that delete_data_content_windows is not needed, so this is removed as
> well.
> ---
> gdb/tui/tui-regs.c | 15 +--------------
> gdb/tui/tui-regs.h |  1 -
> 2 files changed, 1 insertion(+), 15 deletions(-)
>
> diff --git a/gdb/tui/tui-regs.c b/gdb/tui/tui-regs.c
> index f87d1ff8721..9108e460d6f 100644
> --- a/gdb/tui/tui-regs.c
> +++ b/gdb/tui/tui-regs.c
> @@ -269,7 +269,6 @@ tui_data_window::display_registers_from (int start_element_no)
>       /* Create the window if necessary.  */
>       m_regs_content[i].x = box_width () + (m_item_width * j);
>       m_regs_content[i].y = cur_y;
> -      m_regs_content[i].visible = true;
>       m_regs_content[i].rerender (handle.get (), m_item_width);
>       i++;        /* Next register.  */
>     }
> @@ -347,23 +346,13 @@ tui_data_window::first_data_item_displayed ()
> {
>   for (int i = 0; i < m_regs_content.size (); i++)
>     {
> -      if (m_regs_content[i].visible)
> +      if (m_regs_content[i].y > 0)
>     return i;
>     }
>
>   return -1;
> }
>
> -/* See tui-regs.h.  */
> -
> -void
> -tui_data_window::delete_data_content_windows ()
> -{
> -  for (auto &win : m_regs_content)
> -    win.visible = false;
>
> -}
> -
> -
> void
>
> tui_data_window::erase_data_content (const char *prompt)
> {
> @@ -401,7 +390,6 @@ tui_data_window::rerender (bool toplevel)
>   else
>     {
>       erase_data_content (NULL);
> -      delete_data_content_windows ();
>       display_registers_from (0);
>     }
> }
> @@ -426,7 +414,6 @@ tui_data_window::do_scroll_vertical (int num_to_scroll)
>     {
>       first_line += num_to_scroll;
>       erase_data_content (NULL);
> -      delete_data_content_windows ();
>       display_registers_from_line (first_line);
>     }
> }
> diff --git a/gdb/tui/tui-regs.h b/gdb/tui/tui-regs.h
> index 1a9f30fa270..701b8707947 100644
> --- a/gdb/tui/tui-regs.h
> +++ b/gdb/tui/tui-regs.h
> @@ -50,7 +50,6 @@ struct tui_register_info
>   /* The register number.  */
>   int regno;
>   bool highlight = false;
> -  bool visible = false;
>   std::string content;
> };
>
>
> --
> 2.43.0

I think you missed removing tui_data_window::delete_data_content_windows
from tui-regs.h.


Regards
Hannes

Re: [PATCH 11/14] Remove redundant check from tui_refresh_frame_and_register_information

[Hannes Domani]

 Am Sonntag, 17. Dezember 2023, 20:51:57 MEZ hat Tom Tromey <tom@tromey.com> Folgendes geschrieben:

> tui_refresh_frame_and_register_information checks 'from_stack' in a
> block that's already guarded by a 'from_stack' check.  This patch
> removes the redundant check.
> ---
> gdb/tui/tui-hooks.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/gdb/tui/tui-hooks.c b/gdb/tui/tui-hooks.c
> index e47607fefa9..f1e4978a40a 100644
> --- a/gdb/tui/tui-hooks.c
> +++ b/gdb/tui/tui-hooks.c
> @@ -132,11 +132,10 @@ tui_refresh_frame_and_register_information ()
>
>       /* Display the frame position (even if there is no symbols or
>     the PC is not known).  */
> -      bool frame_info_changed_p = tui_show_frame_info (fi);
> +      tui_show_frame_info (fi);
>
>       /* Refresh the register window if it's visible.  */
> -      if (tui_is_window_visible (DATA_WIN)
> -      && (frame_info_changed_p || from_stack))
> +      if (tui_is_window_visible (DATA_WIN))
>      TUI_DATA_WIN->check_register_values (fi);
>     }
>   else if (!from_stack)
>
> --
> 2.43.0

Was the return value of tui_show_frame_info(fi) also useless?


Regards
Hannes

Re: [PATCH 05/14] Remove tui_register_info::visible

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

> tui_register_info::visible is redundant with the fact that y==0 means
> that the register is not visible -- and some spots already check the
> latter.

I'm not sure moving to 'y > 0' is an improvement though...

Would a better solution be to change the visible to a member function,
defined in the header file (so it can inline)?

Thanks,
Andrew


>          This patch removes this member in favor of having a single
> indication of the register's visibility.  This change makes it clear
> that delete_data_content_windows is not needed, so this is removed as
> well.
> ---
>  gdb/tui/tui-regs.c | 15 +--------------
>  gdb/tui/tui-regs.h |  1 -
>  2 files changed, 1 insertion(+), 15 deletions(-)
>
> diff --git a/gdb/tui/tui-regs.c b/gdb/tui/tui-regs.c
> index f87d1ff8721..9108e460d6f 100644
> --- a/gdb/tui/tui-regs.c
> +++ b/gdb/tui/tui-regs.c
> @@ -269,7 +269,6 @@ tui_data_window::display_registers_from (int start_element_no)
>  	  /* Create the window if necessary.  */
>  	  m_regs_content[i].x = box_width () + (m_item_width * j);
>  	  m_regs_content[i].y = cur_y;
> -	  m_regs_content[i].visible = true;
>  	  m_regs_content[i].rerender (handle.get (), m_item_width);
>  	  i++;		/* Next register.  */
>  	}
> @@ -347,23 +346,13 @@ tui_data_window::first_data_item_displayed ()
>  {
>    for (int i = 0; i < m_regs_content.size (); i++)
>      {
> -      if (m_regs_content[i].visible)
> +      if (m_regs_content[i].y > 0)
>  	return i;
>      }
>  
>    return -1;
>  }
>  
> -/* See tui-regs.h.  */
> -
> -void
> -tui_data_window::delete_data_content_windows ()
> -{
> -  for (auto &win : m_regs_content)
> -    win.visible = false;
> -}
> -
> -
>  void
>  tui_data_window::erase_data_content (const char *prompt)
>  {
> @@ -401,7 +390,6 @@ tui_data_window::rerender (bool toplevel)
>    else
>      {
>        erase_data_content (NULL);
> -      delete_data_content_windows ();
>        display_registers_from (0);
>      }
>  }
> @@ -426,7 +414,6 @@ tui_data_window::do_scroll_vertical (int num_to_scroll)
>      {
>        first_line += num_to_scroll;
>        erase_data_content (NULL);
> -      delete_data_content_windows ();
>        display_registers_from_line (first_line);
>      }
>  }
> diff --git a/gdb/tui/tui-regs.h b/gdb/tui/tui-regs.h
> index 1a9f30fa270..701b8707947 100644
> --- a/gdb/tui/tui-regs.h
> +++ b/gdb/tui/tui-regs.h
> @@ -50,7 +50,6 @@ struct tui_register_info
>    /* The register number.  */
>    int regno;
>    bool highlight = false;
> -  bool visible = false;
>    std::string content;
>  };
>  
>
> -- 
> 2.43.0

Re: [PATCH 06/14] Move scrollok call in register window

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

> The register window calls scrollok each time a register is written to
> the window.  However, we only need to call this once, at the start of
> display.  (We could actually call it just once when the window is
> made, but that would involve making another method virtual

Did you consider overriding make_window in the tui_data_window class?
Like this:

  void
  tui_data_window::make_window ()
  {
    tui_win_info::make_window ();
  
    /* In case the regs window is not boxed, we'll write the last char in the
       last line here, causing a scroll, so prevent that.  */
   if (this->handle != nullptr)
      scrollok (this->handle.get (), FALSE);
  }

Thanks,
Andrew




>                                                             or adding a
> new member -- both which I think are worse than this approach.)
> ---
>  gdb/tui/tui-regs.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/gdb/tui/tui-regs.c b/gdb/tui/tui-regs.c
> index 9108e460d6f..0ad23e93778 100644
> --- a/gdb/tui/tui-regs.c
> +++ b/gdb/tui/tui-regs.c
> @@ -237,6 +237,10 @@ tui_data_window::update_register_data (const reggroup *group,
>  void
>  tui_data_window::display_registers_from (int start_element_no)
>  {
> +  /* In case the regs window is not boxed, we'll write the last char in the
> +     last line here, causing a scroll, so prevent that.  */
> +  scrollok (handle.get (), FALSE);
> +
>    int max_len = 0;
>    for (auto &&data_item_win : m_regs_content)
>      {
> @@ -449,10 +453,6 @@ tui_data_window::check_register_values (frame_info_ptr frame)
>  void
>  tui_register_info::rerender (WINDOW *handle, int field_width)
>  {
> -  /* In case the regs window is not boxed, we'll write the last char in the
> -     last line here, causing a scroll, so prevent that.  */
> -  scrollok (handle, FALSE);
> -
>    if (highlight)
>      /* We ignore the return value, casting it to void in order to avoid
>         a compiler warning.  The warning itself was introduced by a patch
>
> -- 
> 2.43.0

Re: [PATCH 07/14] Simplify update_register_data

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

> This changes update_register_data to always update the register data.
> The idea here is that this is really only called when either the
> desired register group changes, or when gdb transitions from not
> having a frame to having a frame.
>
> show_registers is also simplified slightly to account for this.
> ---
>  gdb/tui/tui-regs.c | 57 ++++++++++++++++++++----------------------------------
>  1 file changed, 21 insertions(+), 36 deletions(-)
>
> diff --git a/gdb/tui/tui-regs.c b/gdb/tui/tui-regs.c
> index 0ad23e93778..7b6b669fe51 100644
> --- a/gdb/tui/tui-regs.c
> +++ b/gdb/tui/tui-regs.c
> @@ -170,15 +170,10 @@ tui_data_window::show_registers (const reggroup *group)
>      group = general_reggroup;
>  
>    if (target_has_registers () && target_has_stack () && target_has_memory ())
> -    {
> -      update_register_data (group, get_selected_frame (NULL));
> -
> -      /* Clear all notation of changed values.  */
> -      for (auto &&data_item_win : m_regs_content)
> -	data_item_win.highlight = false;
> -    }
> +    update_register_data (group, get_selected_frame (nullptr));
>    else
>      {
> +      set_title (_("Registers"));

It's strange that you add this line, but in the very next commit change
the text to the empty string.  Maybe I'm missing something, but it feels
like you could just set the empty string here maybe?

Thanks,
Andrew


>        m_current_group = nullptr;
>        m_regs_content.clear ();
>      }
> @@ -195,40 +190,30 @@ void
>  tui_data_window::update_register_data (const reggroup *group,
>  				       frame_info_ptr frame)
>  {
> -  if (group != m_current_group)
> -    {
> -      m_current_group = group;
> +  m_current_group = group;
>  
> -      /* Make a new title showing which group we display.  */
> -      this->set_title (string_printf ("Register group: %s", group->name ()));
> +  /* Make a new title showing which group we display.  */
> +  this->set_title (string_printf ("Register group: %s", group->name ()));
>  
> -      /* Create the registers.  */
> -      m_regs_content.clear ();
> +  /* Create the registers.  */
> +  m_regs_content.clear ();
>  
> -      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;
> +  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.  */
> -	  const char *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;
>  
> -	  m_regs_content.emplace_back (regnum, frame);
> -	}
> -    }
> -  else
> -    {
> -      /* The group did not change, so we can simply update each
> -	 item. */
> -      for (tui_register_info &reg : m_regs_content)
> -	reg.update (frame);
> +      m_regs_content.emplace_back (regnum, frame);
>      }
>  }
>  
>
> -- 
> 2.43.0

Re: [PATCH 08/14] Remove the TUI register window rerender overload

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

> After these restructurings, it should be clear that the rerender
> overload can be removed from the TUI register window.  This is done by
> moving a bit more logic from show_registers into update_register_data.
> After this, update_register_data simply updates the internal state,
> and rerender will write to the screen.  All the actual rendering work
> is done, ultimately, by display_registers_from.  This split between
> updating the mode and rendering makes it clear that the recursive case
> can't happen any longer.
> ---
>  gdb/tui/tui-regs.c | 55 ++++++++++++++++++++++++------------------------------
>  gdb/tui/tui-regs.h | 14 ++++++--------
>  2 files changed, 30 insertions(+), 39 deletions(-)
>
> diff --git a/gdb/tui/tui-regs.c b/gdb/tui/tui-regs.c
> index 7b6b669fe51..5726dc3fea8 100644
> --- a/gdb/tui/tui-regs.c
> +++ b/gdb/tui/tui-regs.c
> @@ -165,31 +165,33 @@ tui_data_window::first_reg_element_no_inline (int line_no) const
>     and refresh the window.  */
>  void
>  tui_data_window::show_registers (const reggroup *group)
> +{
> +  update_register_data (group);
> +  rerender ();
> +}
> +
> +/* Set the data window to display the registers of the register group
> +   using the given frame.  Values are refreshed only when
> +   refresh_values_only is true.  */
> +
> +void
> +tui_data_window::update_register_data (const reggroup *group)

Function comment is not correct.

>  {
>    if (group == nullptr)
>      group = general_reggroup;
>  
> -  if (target_has_registers () && target_has_stack () && target_has_memory ())
> -    update_register_data (group, get_selected_frame (nullptr));
> -  else
> +  if (!target_has_registers ()
> +      || !target_has_stack ()
> +      || !target_has_memory ())
>      {
> -      set_title (_("Registers"));
> +      set_title ("");

This is the title change I pointed out in the previous commit.  I
actually like the 'Registers' string as it means the window always has a
title, rather than the title appearing  / disappearing.

Thanks,
Andrew
>        m_current_group = nullptr;
>        m_regs_content.clear ();
> +      return;
>      }
>  
> -  rerender (false);
> -}
> -
> -
> -/* Set the data window to display the registers of the register group
> -   using the given frame.  Values are refreshed only when
> -   refresh_values_only is true.  */
> +  frame_info_ptr frame = get_selected_frame (nullptr);
>  
> -void
> -tui_data_window::update_register_data (const reggroup *group,
> -				       frame_info_ptr frame)
> -{
>    m_current_group = group;
>  
>    /* Make a new title showing which group we display.  */
> @@ -222,6 +224,9 @@ tui_data_window::update_register_data (const reggroup *group,
>  void
>  tui_data_window::display_registers_from (int start_element_no)
>  {
> +  werase (handle.get ());
> +  check_and_display_highlight_if_needed ();
> +
>    /* In case the regs window is not boxed, we'll write the last char in the
>       last line here, causing a scroll, so prevent that.  */
>    scrollok (handle.get (), FALSE);
> @@ -358,29 +363,18 @@ tui_data_window::erase_data_content (const char *prompt)
>  	x_pos = half_width - strlen (prompt);
>        display_string (height / 2, x_pos, prompt);
>      }
> -  tui_wrefresh (handle.get ());
>  }
>  
>  /* See tui-regs.h.  */
>  
>  void
> -tui_data_window::rerender (bool toplevel)
> +tui_data_window::rerender ()
>  {
>    if (m_regs_content.empty ())
> -    {
> -      if (toplevel && has_stack_frames ())
> -	{
> -	  frame_info_ptr fi = get_selected_frame (NULL);
> -	  check_register_values (fi);
> -	}
> -      else
> -	erase_data_content (_("[ Register Values Unavailable ]"));
> -    }
> +    erase_data_content (_("[ Register Values Unavailable ]"));
>    else
> -    {
> -      erase_data_content (NULL);
> -      display_registers_from (0);
> -    }
> +    display_registers_from (0);
> +  tui_wrefresh (handle.get ());
>  }
>  
>  
> @@ -402,7 +396,6 @@ tui_data_window::do_scroll_vertical (int num_to_scroll)
>    if (first_line >= 0)
>      {
>        first_line += num_to_scroll;
> -      erase_data_content (NULL);
>        display_registers_from_line (first_line);
>      }
>  }
> diff --git a/gdb/tui/tui-regs.h b/gdb/tui/tui-regs.h
> index 701b8707947..b31a5a3e5b8 100644
> --- a/gdb/tui/tui-regs.h
> +++ b/gdb/tui/tui-regs.h
> @@ -56,7 +56,10 @@ struct tui_register_info
>  /* The TUI registers window.  */
>  struct tui_data_window : public tui_win_info
>  {
> -  tui_data_window () = default;
> +  tui_data_window ()
> +  {
> +    update_register_data (nullptr);
> +  }
>  
>    DISABLE_COPY_AND_ASSIGN (tui_data_window);
>  
> @@ -81,11 +84,7 @@ struct tui_data_window : public tui_win_info
>    {
>    }
>  
> -  void rerender (bool toplevel);
> -  void rerender () override
> -  {
> -    rerender (true);
> -  }
> +  void rerender () override;
>  
>  private:
>  
> @@ -110,8 +109,7 @@ 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 update_register_data (const reggroup *group,
> -			     frame_info_ptr frame);
> +  void update_register_data (const reggroup *group);
>  
>    /* Answer the number of the last line in the regs display.  If there
>       are no registers (-1) is returned.  */
>
> -- 
> 2.43.0

Re: [PATCH 06/14] Move scrollok call in register window

[Tom Tromey]

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

Andrew> Tom Tromey <tom@tromey.com> writes:
>> The register window calls scrollok each time a register is written to
>> the window.  However, we only need to call this once, at the start of
>> display.  (We could actually call it just once when the window is
>> made, but that would involve making another method virtual

Andrew> Did you consider overriding make_window in the tui_data_window class?
Andrew> Like this:

I did, but there aren't any other overrides of make_window, and it seems
like a mistake that it is virtual at all.

Tom

Re: [PATCH 11/14] Remove redundant check from tui_refresh_frame_and_register_information

[Tom Tromey]

>>>>> "Hannes" == Hannes Domani <ssbssa@yahoo.de> writes:

>> -      if (tui_is_window_visible (DATA_WIN)
>> -      && (frame_info_changed_p || from_stack))

Hannes> Was the return value of tui_show_frame_info(fi) also useless?

Yes, in the above, 'from_stack' is always true, so the value of
'frame_info_changed_p' is immaterial.

Tom

Re: [PATCH 11/14] Remove redundant check from tui_refresh_frame_and_register_information

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

>>>>>> "Hannes" == Hannes Domani <ssbssa@yahoo.de> writes:
>
>>> -      if (tui_is_window_visible (DATA_WIN)
>>> -      && (frame_info_changed_p || from_stack))
>
> Hannes> Was the return value of tui_show_frame_info(fi) also useless?
>
> Yes, in the above, 'from_stack' is always true, so the value of
> 'frame_info_changed_p' is immaterial.

Except I think that's a bug in the original code.

As your previous patch points out we already hook the register changed
observer, so the only time that a register change will need this
function to refresh the register display is if the frame was changed.

If we're going to stop checking frame_info_changed_p then we might as
well stop hooking gdb::observers::register_changed and just rely on the
before prompt hook to update everything, right?

Thanks,
Andrew

Re: [PATCH 00/14] Cleanups for the TUi register window

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

> This series is a grab-bag of cleanups to the TUI register window.
>
> I've tried to untangle the code somewhat, with the goal being removing
> the extra rerender overload.
>
> This series also fixes the 'exit' bug and removes the hacky and
> unnecessary recursion flag from tui-hooks.c.
>
> More cleanups here are possible -- the register window is pretty old
> code and pretty ugly.  In particular:
>
> * the layout code is still pretty bad
>
> * it doesn't really make sense for check_register_values to accept a
>   frame, because the window can only really ever use the selected
>   frame anyway
>
> * there's no horizontal scrolling, but for vector registers this might
>   be nice (there's a bug about this)

Thanks.  A nice set of improvements.  Read through the series, replied
where I had feedback.

Reviewed-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew



>
> ---
> Tom Tromey (14):
>       Use pop_back in tui_register_format
>       Minor C++ cleanups in tui-regs.c
>       Simplify tui_data_window::show_register_group
>       Rename tui_data_item_window -> tui_register_info
>       Remove tui_register_info::visible
>       Move scrollok call in register window
>       Simplify update_register_data
>       Remove the TUI register window rerender overload
>       Simplify tui_data_win::erase_data_content
>       Remove tui_refreshing_registers
>       Remove redundant check from tui_refresh_frame_and_register_information
>       Return void from tui_show_frame_info
>       Rename show_registers -> set_register_group
>       Update TUI register window when the inferior exits
>
>  gdb/testsuite/gdb.tui/regs.exp |   8 ++
>  gdb/tui/tui-hooks.c            |  36 +++----
>  gdb/tui/tui-regs.c             | 220 +++++++++++++++--------------------------
>  gdb/tui/tui-regs.h             |  47 +++++----
>  gdb/tui/tui-stack.c            |  16 ++-
>  gdb/tui/tui-stack.h            |   2 +-
>  6 files changed, 137 insertions(+), 192 deletions(-)
> ---
> base-commit: 2757c1c65fd6ba10c55ba5cf38d600814cf9dc1b
> change-id: 20231217-tui-regs-cleanup-36d8f390a65a
>
> Best regards,
> -- 
> Tom Tromey <tom@tromey.com>

Re: [PATCH 05/14] Remove tui_register_info::visible

[Tom Tromey]

>>>>> "Hannes" == Hannes Domani <ssbssa@yahoo.de> writes:

Hannes>  Am Sonntag, 17. Dezember 2023, 20:51:14 MEZ hat Tom Tromey <tom@tromey.com> Folgendes geschrieben:
>> tui_register_info::visible is redundant with the fact that y==0 means
>> that the register is not visible -- and some spots already check the
>> latter.  This patch removes this member in favor of having a single
>> indication of the register's visibility.  This change makes it clear
>> that delete_data_content_windows is not needed, so this is removed as
>> well.

Hannes> I think you missed removing tui_data_window::delete_data_content_windows
Hannes> from tui-regs.h.

Thanks, I fixed this.

Tom

Re: [PATCH 05/14] Remove tui_register_info::visible

[Tom Tromey]

>> tui_register_info::visible is redundant with the fact that y==0 means
>> that the register is not visible -- and some spots already check the
>> latter.

Andrew> I'm not sure moving to 'y > 0' is an improvement though...

Andrew> Would a better solution be to change the visible to a member function,
Andrew> defined in the header file (so it can inline)?

I made this change.

Tom

Re: [PATCH 07/14] Simplify update_register_data

[Tom Tromey]

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

>> +      set_title (_("Registers"));

Andrew> It's strange that you add this line, but in the very next commit change
Andrew> the text to the empty string.  Maybe I'm missing something, but it feels
Andrew> like you could just set the empty string here maybe?

I think I meant to fix this up and forgot.  Anyway I've done it now.

Tom

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

[Tom Tromey]

>>>>> "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

Re: [PATCH 08/14] Remove the TUI register window rerender overload

[Tom Tromey]

>> -      set_title (_("Registers"));
>> +      set_title ("");

Andrew> This is the title change I pointed out in the previous commit.  I
Andrew> actually like the 'Registers' string as it means the window always has a
Andrew> title, rather than the title appearing  / disappearing.

I went ahead & left it in.

Tom

Re: [PATCH 11/14] Remove redundant check from tui_refresh_frame_and_register_information

[Tom Tromey]

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

>> Yes, in the above, 'from_stack' is always true, so the value of
>> 'frame_info_changed_p' is immaterial.

Andrew> Except I think that's a bug in the original code.

Andrew> As your previous patch points out we already hook the register changed
Andrew> observer, so the only time that a register change will need this
Andrew> function to refresh the register display is if the frame was changed.

Andrew> If we're going to stop checking frame_info_changed_p then we might as
Andrew> well stop hooking gdb::observers::register_changed and just rely on the
Andrew> before prompt hook to update everything, right?

I don't think that will work correctly, because the TUI register window
shows changed registers in a special way.  If we rely solely on the
before-prompt hook, then any command at all will cause a redisplay that
will erase the "changed" highlighting.

This is why two phases are needed: the register window hooks into the
various things that can cause a register change, and then this is
checked before redisplay.

The patch in question here just removes some code that checks if the
frame changed.  However, if the frame did change, from_stack is already
set due to the context-changed observer.

Tom