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

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

[Tom de Vries]

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    | 85 ++++++++++++++++++++++++++++++++++++++++---
 gdb/tui/tui-stack.h   | 34 ++---------------
 3 files changed, 86 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 a1e44f77cc1..37d566c0186 100644
--- a/gdb/tui/tui-data.h
+++ b/gdb/tui/tui-data.h
@@ -182,6 +182,76 @@ 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"
@@ -192,11 +262,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 \
+  (dynamic_cast<tui_source_window *> (tui_win_list[SRC_WIN]))
+#define TUI_DISASM_WIN \
+  (dynamic_cast<tui_disasm_window *> (tui_win_list[DISASSEM_WIN]))
+#define TUI_DATA_WIN \
+  (dynamic_cast<tui_data_window *> (tui_win_list[DATA_WIN]))
+#define TUI_CMD_WIN \
+  (dynamic_cast<tui_cmd_window *> (tui_win_list[CMD_WIN]))
+#define TUI_STATUS_WIN \
+  (dynamic_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: 328e01595436e937a0cc00cc8451f17beda26799
-- 
2.35.3

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

[Tom de Vries]

On 11/12/23 16:06, Tom de Vries wrote:
> +struct tui_cmd_window
> + : public tui_noscroll_window, tui_nobox_window, 
> tui_norefresh_window, + tui_always_visible_window

Evidently, this introduces multiple inheritance.

Here ( https://gcc.gnu.org/codingconventions.html#CandCxx ) we read:
...
  Complex hierarchies are to be avoided. Take special care with multiple 
inheritance. On the rare occasion that using mulitple inheritance is 
indeed useful, prepare design rationales in advance, and take special 
care to make documentation of the entire hierarchy clear. (In 
particular, multiple inheritance can be an acceptable way of combining 
"traits"-style classes that only contain static member functions. Its 
use with data-carrying classes is more problematic.)
...

I think the newly introduced classes are close enough to the traits 
concept described above for the multiple inheritance to be acceptable.

Thanks,
- Tom

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

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> Evidently, this introduces multiple inheritance.

Tom> Here ( https://gcc.gnu.org/codingconventions.html#CandCxx ) we read:

FWIW you can't really go by the GCC conventions.  GCC is a lot more
conservative about C++ than GDB is.

Tom> I think the newly introduced classes are close enough to the traits
Tom> concept described above for the multiple inheritance to be acceptable.

Makes sense to me.  And FWIW GDB already uses multiple inheritance.

Tom

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

[Tom de Vries]

On 11/13/23 18:09, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> Evidently, this introduces multiple inheritance.
> 
> Tom> Here ( https://gcc.gnu.org/codingconventions.html#CandCxx ) we read:
> 
> FWIW you can't really go by the GCC conventions.  GCC is a lot more
> conservative about C++ than GDB is.
> 

I came across it via 
https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#C.2B-.2B--specific_coding_conventions 
:
...
C++-specific coding conventions

GDB follows GCC's C++ coding conventions, diverging only on finer 
detail, as we find necessary.

See these documents, which have extensive detail:

     GCC's C++ Language Conventions (scroll down a bit)

     GCC Coding Conventions Rationale and Discussion

A point where we diverge is in permitting the use of C++ exceptions.

...

So I suppose this is one of those finer details ;)

Thanks,
- Tom

> Tom> I think the newly introduced classes are close enough to the traits
> Tom> concept described above for the multiple inheritance to be acceptable.
> 
> Makes sense to me.  And FWIW GDB already uses multiple inheritance.
> 
> Tom

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

[Tom Tromey]

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

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.

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.

Tom

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

[Tom de Vries]

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

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

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

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

It's fine by me.
Approved-By: Tom Tromey <tom@tromey.com>

Tom

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

[Simon Marchi]

On 11/13/23 12: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.
> 
> 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.
> 
> 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.

Actually, since the base is virtual, dynamic_cast is needed.  Try
changing development to false in bfd/development.sh, you'll get:

      CXX    tui/tui-layout.o
    In file included from /home/smarchi/src/binutils-gdb/gdb/tui/tui-data.h:31,
                     from /home/smarchi/src/binutils-gdb/gdb/tui/tui-command.h:25,
                     from /home/smarchi/src/binutils-gdb/gdb/tui/tui-layout.c:31:
    /home/smarchi/src/binutils-gdb/gdb/../gdbsupport/gdb-checked-static-cast.h: In instantiation of ‘T gdb::checked_static_cast(V*) [with T = tui_cmd_window*; V = tui_win_info]’:
    /home/smarchi/src/binutils-gdb/gdb/tui/tui-layout.c:79:3:   required from here
    /home/smarchi/src/binutils-gdb/gdb/../gdbsupport/gdb-checked-static-cast.h:64:14: error: cannot convert from pointer to base class ‘tui_win_info’ to pointer to derived class ‘tui_cmd_window’ because the base is virtual
       64 |   T result = static_cast<T> (v);
          |              ^~~~~~~~~~~~~~~~~~

It might be a good idea to add a static_cast in the DEVELOPMENT branch
of checked_static_cast to catch this kind of mistake.  I think it could
be written this way to be succinct (but untested):


      T result = static_cast<T> (v);
    #ifdef DEVELOPMENT
      gdb_assert (result == dynamic_cast<T> (v));
    #endif

      return result;

IOW, this patch:

diff --git a/gdbsupport/gdb-checked-static-cast.h b/gdbsupport/gdb-checked-static-cast.h
index b6ce8c8bb04..e935bb359f0 100644
--- a/gdbsupport/gdb-checked-static-cast.h
+++ b/gdbsupport/gdb-checked-static-cast.h
@@ -54,14 +54,9 @@ checked_static_cast (V *v)
                 || std::is_base_of<T_no_P, V>::value,
                 "types must be related");

-#ifdef DEVELOPMENT
-  if (v == nullptr)
-    return nullptr;
-
-  T result = dynamic_cast<T> (v);
-  gdb_assert (result != nullptr);
-#else
   T result = static_cast<T> (v);
+#ifdef DEVELOPMENT
+  gdb_assert (result == dynamic_cast<T> (v));
 #endif

   return result;

Simon

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

[Tom de Vries]

On 2/21/24 18:32, Simon Marchi wrote:
> On 11/13/23 12: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.
>>
>> 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.
>>
>> 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.
> 
> Actually, since the base is virtual, dynamic_cast is needed.  Try
> changing development to false in bfd/development.sh, you'll get:
> 
>        CXX    tui/tui-layout.o
>      In file included from /home/smarchi/src/binutils-gdb/gdb/tui/tui-data.h:31,
>                       from /home/smarchi/src/binutils-gdb/gdb/tui/tui-command.h:25,
>                       from /home/smarchi/src/binutils-gdb/gdb/tui/tui-layout.c:31:
>      /home/smarchi/src/binutils-gdb/gdb/../gdbsupport/gdb-checked-static-cast.h: In instantiation of ‘T gdb::checked_static_cast(V*) [with T = tui_cmd_window*; V = tui_win_info]’:
>      /home/smarchi/src/binutils-gdb/gdb/tui/tui-layout.c:79:3:   required from here
>      /home/smarchi/src/binutils-gdb/gdb/../gdbsupport/gdb-checked-static-cast.h:64:14: error: cannot convert from pointer to base class ‘tui_win_info’ to pointer to derived class ‘tui_cmd_window’ because the base is virtual
>         64 |   T result = static_cast<T> (v);
>            |              ^~~~~~~~~~~~~~~~~~
> 
> It might be a good idea to add a static_cast in the DEVELOPMENT branch
> of checked_static_cast to catch this kind of mistake.  I think it could
> be written this way to be succinct (but untested):
> 
> 
>        T result = static_cast<T> (v);
>      #ifdef DEVELOPMENT
>        gdb_assert (result == dynamic_cast<T> (v));
>      #endif
> 
>        return result;
> 
> IOW, this patch:
> 

Hi Simon,

LGTM.  Also I can confirm that the patch catches the problem show above 
(which was reported as PR31399).

I'll write a fix for that PR, and test with your patch applied as well.

Thanks,
- Tom


> diff --git a/gdbsupport/gdb-checked-static-cast.h b/gdbsupport/gdb-checked-static-cast.h
> index b6ce8c8bb04..e935bb359f0 100644
> --- a/gdbsupport/gdb-checked-static-cast.h
> +++ b/gdbsupport/gdb-checked-static-cast.h
> @@ -54,14 +54,9 @@ checked_static_cast (V *v)
>                   || std::is_base_of<T_no_P, V>::value,
>                   "types must be related");
> 
> -#ifdef DEVELOPMENT
> -  if (v == nullptr)
> -    return nullptr;
> -
> -  T result = dynamic_cast<T> (v);
> -  gdb_assert (result != nullptr);
> -#else
>     T result = static_cast<T> (v);
> +#ifdef DEVELOPMENT
> +  gdb_assert (result == dynamic_cast<T> (v));
>   #endif
> 
>     return result;
> 
> Simon

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

[Tom de Vries]

On 2/22/24 13:07, Tom de Vries wrote:
> On 2/21/24 18:32, Simon Marchi wrote:
>> On 11/13/23 12: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.
>>>
>>> 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.
>>>
>>> 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.
>>
>> Actually, since the base is virtual, dynamic_cast is needed.  Try
>> changing development to false in bfd/development.sh, you'll get:
>>
>>        CXX    tui/tui-layout.o
>>      In file included from 
>> /home/smarchi/src/binutils-gdb/gdb/tui/tui-data.h:31,
>>                       from 
>> /home/smarchi/src/binutils-gdb/gdb/tui/tui-command.h:25,
>>                       from 
>> /home/smarchi/src/binutils-gdb/gdb/tui/tui-layout.c:31:
>>      
>> /home/smarchi/src/binutils-gdb/gdb/../gdbsupport/gdb-checked-static-cast.h: In instantiation of ‘T gdb::checked_static_cast(V*) [with T = tui_cmd_window*; V = tui_win_info]’:
>>      /home/smarchi/src/binutils-gdb/gdb/tui/tui-layout.c:79:3:   
>> required from here
>>      
>> /home/smarchi/src/binutils-gdb/gdb/../gdbsupport/gdb-checked-static-cast.h:64:14: error: cannot convert from pointer to base class ‘tui_win_info’ to pointer to derived class ‘tui_cmd_window’ because the base is virtual
>>         64 |   T result = static_cast<T> (v);
>>            |              ^~~~~~~~~~~~~~~~~~
>>
>> It might be a good idea to add a static_cast in the DEVELOPMENT branch
>> of checked_static_cast to catch this kind of mistake.  I think it could
>> be written this way to be succinct (but untested):
>>
>>
>>        T result = static_cast<T> (v);
>>      #ifdef DEVELOPMENT
>>        gdb_assert (result == dynamic_cast<T> (v));
>>      #endif
>>
>>        return result;
>>
>> IOW, this patch:
>>
> 
> Hi Simon,
> 
> LGTM.  Also I can confirm that the patch catches the problem show above 
> (which was reported as PR31399).
> 
> I'll write a fix for that PR, and test with your patch applied as well.
> 

A note on the checked_static_cast patch, or rather the static_cast it self.

It catches the problem at build time for TUI_STATUS_WIN and TUI_CMD_WIN, 
but not for the other 3.

I think the problem exists though for all 5 windows (tui_win_info is a 
virtual base of all 5 windows), so I'm gonna use dynamic_cast for all of 
them.

IWBN to catch the other 3 cases as well, but I'm not sure if or how this 
can be done.

Thanks,
- Tom

> Thanks,
> - Tom
> 
> 
>> diff --git a/gdbsupport/gdb-checked-static-cast.h 
>> b/gdbsupport/gdb-checked-static-cast.h
>> index b6ce8c8bb04..e935bb359f0 100644
>> --- a/gdbsupport/gdb-checked-static-cast.h
>> +++ b/gdbsupport/gdb-checked-static-cast.h
>> @@ -54,14 +54,9 @@ checked_static_cast (V *v)
>>                   || std::is_base_of<T_no_P, V>::value,
>>                   "types must be related");
>>
>> -#ifdef DEVELOPMENT
>> -  if (v == nullptr)
>> -    return nullptr;
>> -
>> -  T result = dynamic_cast<T> (v);
>> -  gdb_assert (result != nullptr);
>> -#else
>>     T result = static_cast<T> (v);
>> +#ifdef DEVELOPMENT
>> +  gdb_assert (result == dynamic_cast<T> (v));
>>   #endif
>>
>>     return result;
>>
>> Simon
> 

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

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> It catches the problem at build time for TUI_STATUS_WIN and
Tom> TUI_CMD_WIN, but not for the other 3.

Those don't have a problem because they don't use virtual inheritance.

The problem comes from any class using one of the mixins like:

struct tui_always_visible_window : public virtual tui_win_info

thanks,
Tom

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

[Tom de Vries]

On 2/22/24 16:07, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> It catches the problem at build time for TUI_STATUS_WIN and
> Tom> TUI_CMD_WIN, but not for the other 3.
> 
> Those don't have a problem because they don't use virtual inheritance.
> 
> The problem comes from any class using one of the mixins like:
> 
> struct tui_always_visible_window : public virtual tui_win_info
> 

Hi Tom,

Sorry, you're right.  I see I confused virtual base with abstract base.

Thanks,
- Tom

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

[Tom Tromey]

Tom> Sorry, you're right.  I see I confused virtual base with abstract base.

No worries, IMO it's a confusing term since 'virtual' also means
something more normal for methods.

It probably would be somewhat more future-proof to just use dynamic_cast
in all of these macros.

Old-school OO programming would say all these casts are bad and we
should instead work toward expressing these requirements via new
methods.

Tom