Re: [PATCH] [gdb/tui] Factor out tui_noscroll_window et al

[Tom de Vries <tdevries@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 1235 bytes --]

On 11/13/23 18:08, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> +/* A TUI window that doesn't scroll.  */
> Tom> +
> Tom> +struct tui_noscroll_window : public virtual tui_win_info {
> 
> The "{" should be on the next line.
> 

Done.

> Tom> +/* A TUI window that occupies a single line.  */
> Tom> +
> Tom> +struct tui_oneline_window : public virtual tui_win_info {
> 
> Some of these only have a single use.  That seems like overkill to me,
> unless we're introducing another use someday.
> 

I understand that position, but I prefer this, even if there's one use 
(so, I've left it as is for now, if you object I'll drop the single use 
ones).

In particular, I like how much shorter some window declaration are, it 
makes them easier to read imo.  I think this style moves the boiler 
plate code out of the way and makes the window declaration more focused 
on the specifics of what it's trying to achieve.

> Tom> +#define TUI_SRC_WIN \
> Tom> +  (dynamic_cast<tui_source_window *> (tui_win_list[SRC_WIN]))
> Tom> +#define TUI_DISASM_WIN \
> Tom> +  (dynamic_cast<tui_disasm_window *> (tui_win_list[DISASSEM_WIN]))
> 
> I think these should use gdb::checked_static_cast.

Done.

Thanks,
- Tom

[-- Attachment #2: 0001-gdb-tui-Factor-out-tui_noscroll_window-et-al.patch --]
[-- Type: text/x-patch, Size: 6745 bytes --]

From c5b0825aa3894d71c706e989018b5ff5b1aa26da Mon Sep 17 00:00:00 2001
From: Tom de Vries <tdevries@suse.de>
Date: Sun, 12 Nov 2023 11:10:22 +0100
Subject: [PATCH] [gdb/tui] Factor out tui_noscroll_window et al

I noticed that tui_locator_window has an empty do_scroll_vertical and
do_scroll_horizontal, like tui_cmd_window, but unlike tui_cmd_window doesn't
have:
...
  bool can_scroll () const override
  {
    return false;
  }
...

I suspect that it probably doesn't matter, but regardless it's good to have
the same implementations of basic properties in all windows.

Ensure this by adding a class tui_noscroll_window, that has:
- an empty do_scroll_vertical and do_scroll_horizontal, and
- a can_scroll returning false
which both tui_locator_window and tui_cmd_window inherit.

Make all methods final to ensure no accidental overrides are left in the
inheriting classes.

Likewise add new classes representing basic window properties:
- tui_nofocus_window,
- tui_oneline_window,
- tui_nobox_window,
- tui_norefresh_window, and
- tui_always_visible_window.

The changes are only a refactoring, apart from adding the "final", which does
limit the range of behaviours for subclasses.

Tested on x86_64-linux.
---
 gdb/tui/tui-command.h | 33 ++--------------
 gdb/tui/tui-data.h    | 92 ++++++++++++++++++++++++++++++++++++++++---
 gdb/tui/tui-stack.h   | 34 ++--------------
 3 files changed, 93 insertions(+), 66 deletions(-)

diff --git a/gdb/tui/tui-command.h b/gdb/tui/tui-command.h
index f6842880bb2..d743dd87a1b 100644
--- a/gdb/tui/tui-command.h
+++ b/gdb/tui/tui-command.h
@@ -25,49 +25,22 @@
 #include "tui/tui-data.h"
 
 /* The TUI command window.  */
-struct tui_cmd_window : public tui_win_info
+struct tui_cmd_window
+  : public tui_noscroll_window, tui_nobox_window, tui_norefresh_window,
+    tui_always_visible_window
 {
   tui_cmd_window () = default;
 
   DISABLE_COPY_AND_ASSIGN (tui_cmd_window);
 
-  void refresh_window () override
-  {
-  }
-
   const char *name () const override
   {
     return CMD_NAME;
   }
 
-  bool can_scroll () const override
-  {
-    return false;
-  }
-
-  bool can_box () const override
-  {
-    return false;
-  }
-
   void resize (int height, int width, int origin_x, int origin_y) override;
 
-  void make_visible (bool visible) override
-  {
-    /* The command window can't be made invisible.  */
-  }
-
   int start_line = 0;
-
-protected:
-
-  void do_scroll_vertical (int num_to_scroll) override
-  {
-  }
-
-  void do_scroll_horizontal (int num_to_scroll) override
-  {
-  }
 };
 
 /* Refresh the command window.  */
diff --git a/gdb/tui/tui-data.h b/gdb/tui/tui-data.h
index 5bb5ef9d941..82340e594ee 100644
--- a/gdb/tui/tui-data.h
+++ b/gdb/tui/tui-data.h
@@ -25,6 +25,7 @@
 #include "tui/tui.h"
 #include "gdb_curses.h"
 #include "observable.h"
+#include "gdbsupport/gdb-checked-static-cast.h"
 
 /* A deleter that calls delwin.  */
 struct curses_deleter
@@ -194,6 +195,82 @@ struct tui_win_info
   std::string m_title;
 };
 
+/* A TUI window that doesn't scroll.  */
+
+struct tui_noscroll_window : public virtual tui_win_info
+{
+public:
+  virtual bool can_scroll () const final override
+  {
+    return false;
+  }
+
+protected:
+  virtual void do_scroll_vertical (int num_to_scroll) final override
+  {
+  }
+
+  /* Scroll the contents horizontally.  This is only called via
+     left_scroll and right_scroll.  */
+  virtual void do_scroll_horizontal (int num_to_scroll) final override
+  {
+  }
+};
+
+/* A TUI window that cannot have focus.  */
+
+struct tui_nofocus_window : public virtual tui_win_info
+{
+public:
+  virtual bool can_focus () const final override
+  {
+    return false;
+  }
+};
+
+/* A TUI window that occupies a single line.  */
+
+struct tui_oneline_window : public virtual tui_win_info
+{
+  int max_height () const final override
+  {
+    return 1;
+  }
+
+  int min_height () const final override
+  {
+    return 1;
+  }
+};
+
+/* A TUI window that has no border.  */
+
+struct tui_nobox_window : public virtual tui_win_info
+{
+  bool can_box () const final override
+  {
+    return false;
+  }
+};
+
+/* A TUI window that is not refreshed.  */
+
+struct tui_norefresh_window : public virtual tui_win_info
+{
+  virtual void refresh_window () final override
+  {
+  }
+};
+
+/* A TUI window that is always visible.  */
+
+struct tui_always_visible_window : public virtual tui_win_info
+{
+  virtual void make_visible (bool visible) final override
+  {
+  }
+};
+
 /* Constant definitions.  */
 #define SRC_NAME                "src"
 #define CMD_NAME                "cmd"
@@ -204,11 +281,16 @@ struct tui_win_info
 /* Global Data.  */
 extern struct tui_win_info *tui_win_list[MAX_MAJOR_WINDOWS];
 
-#define TUI_SRC_WIN     ((tui_source_window *) tui_win_list[SRC_WIN])
-#define TUI_DISASM_WIN	((tui_disasm_window *) tui_win_list[DISASSEM_WIN])
-#define TUI_DATA_WIN    ((tui_data_window *) tui_win_list[DATA_WIN])
-#define TUI_CMD_WIN     ((tui_cmd_window *) tui_win_list[CMD_WIN])
-#define TUI_STATUS_WIN  ((tui_locator_window *) tui_win_list[STATUS_WIN])
+#define TUI_SRC_WIN \
+  (gdb::checked_static_cast<tui_source_window *> (tui_win_list[SRC_WIN]))
+#define TUI_DISASM_WIN \
+  (gdb::checked_static_cast<tui_disasm_window *> (tui_win_list[DISASSEM_WIN]))
+#define TUI_DATA_WIN \
+  (gdb::checked_static_cast<tui_data_window *> (tui_win_list[DATA_WIN]))
+#define TUI_CMD_WIN \
+  (gdb::checked_static_cast<tui_cmd_window *> (tui_win_list[CMD_WIN]))
+#define TUI_STATUS_WIN \
+  (gdb::checked_static_cast<tui_locator_window *> (tui_win_list[STATUS_WIN]))
 
 /* All the windows that are currently instantiated, in layout
    order.  */
diff --git a/gdb/tui/tui-stack.h b/gdb/tui/tui-stack.h
index d542f215771..ca95b2bf78a 100644
--- a/gdb/tui/tui-stack.h
+++ b/gdb/tui/tui-stack.h
@@ -28,7 +28,9 @@ class frame_info_ptr;
 
 /* Locator window class.  */
 
-struct tui_locator_window : public tui_win_info
+struct tui_locator_window
+  : public tui_nofocus_window, tui_noscroll_window, tui_oneline_window,
+    tui_nobox_window
 {
   tui_locator_window () = default;
 
@@ -37,38 +39,8 @@ struct tui_locator_window : public tui_win_info
     return STATUS_NAME;
   }
 
-  int max_height () const override
-  {
-    return 1;
-  }
-
-  int min_height () const override
-  {
-    return 1;
-  }
-
-  bool can_box () const override
-  {
-    return false;
-  }
-
-  bool can_focus () const override
-  {
-    return false;
-  }
-
   void rerender () override;
 
-protected:
-
-  void do_scroll_vertical (int n) override
-  {
-  }
-
-  void do_scroll_horizontal (int n) override
-  {
-  }
-
 private:
 
   /* Create the status line to display as much information as we can

base-commit: 5fa871f5d93bf285753f219cf583d0763dc0cd33
-- 
2.35.3