* [PATCH] [gdb/tui] Factor out tui_noscroll_window et al @ 2023-11-12 15:06 Tom de Vries 2023-11-13 9:37 ` Tom de Vries 2023-11-13 17:08 ` Tom Tromey 0 siblings, 2 replies; 13+ messages in thread From: Tom de Vries @ 2023-11-12 15:06 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] [gdb/tui] Factor out tui_noscroll_window et al 2023-11-12 15:06 [PATCH] [gdb/tui] Factor out tui_noscroll_window et al Tom de Vries @ 2023-11-13 9:37 ` Tom de Vries 2023-11-13 17:09 ` Tom Tromey 2023-11-13 17:08 ` Tom Tromey 1 sibling, 1 reply; 13+ messages in thread From: Tom de Vries @ 2023-11-13 9:37 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] [gdb/tui] Factor out tui_noscroll_window et al 2023-11-13 9:37 ` Tom de Vries @ 2023-11-13 17:09 ` Tom Tromey 2023-11-14 15:02 ` Tom de Vries 0 siblings, 1 reply; 13+ messages in thread From: Tom Tromey @ 2023-11-13 17:09 UTC (permalink / raw) To: Tom de Vries; +Cc: gdb-patches >>>>> "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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] [gdb/tui] Factor out tui_noscroll_window et al 2023-11-13 17:09 ` Tom Tromey @ 2023-11-14 15:02 ` Tom de Vries 0 siblings, 0 replies; 13+ messages in thread From: Tom de Vries @ 2023-11-14 15:02 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] [gdb/tui] Factor out tui_noscroll_window et al 2023-11-12 15:06 [PATCH] [gdb/tui] Factor out tui_noscroll_window et al Tom de Vries 2023-11-13 9:37 ` Tom de Vries @ 2023-11-13 17:08 ` Tom Tromey 2023-11-13 21:03 ` Tom de Vries 2024-02-21 17:32 ` Simon Marchi 1 sibling, 2 replies; 13+ messages in thread From: Tom Tromey @ 2023-11-13 17:08 UTC (permalink / raw) To: Tom de Vries; +Cc: gdb-patches >>>>> "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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] [gdb/tui] Factor out tui_noscroll_window et al 2023-11-13 17:08 ` Tom Tromey @ 2023-11-13 21:03 ` Tom de Vries 2023-11-14 14:13 ` Tom Tromey 2024-02-21 17:32 ` Simon Marchi 1 sibling, 1 reply; 13+ messages in thread From: Tom de Vries @ 2023-11-13 21:03 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches [-- 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] [gdb/tui] Factor out tui_noscroll_window et al 2023-11-13 21:03 ` Tom de Vries @ 2023-11-14 14:13 ` Tom Tromey 0 siblings, 0 replies; 13+ messages in thread From: Tom Tromey @ 2023-11-14 14:13 UTC (permalink / raw) To: Tom de Vries; +Cc: Tom Tromey, gdb-patches >>>>> "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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] [gdb/tui] Factor out tui_noscroll_window et al 2023-11-13 17:08 ` Tom Tromey 2023-11-13 21:03 ` Tom de Vries @ 2024-02-21 17:32 ` Simon Marchi 2024-02-22 12:07 ` Tom de Vries 1 sibling, 1 reply; 13+ messages in thread From: Simon Marchi @ 2024-02-21 17:32 UTC (permalink / raw) To: Tom Tromey, Tom de Vries; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] [gdb/tui] Factor out tui_noscroll_window et al 2024-02-21 17:32 ` Simon Marchi @ 2024-02-22 12:07 ` Tom de Vries 2024-02-22 13:13 ` Tom de Vries 0 siblings, 1 reply; 13+ messages in thread From: Tom de Vries @ 2024-02-22 12:07 UTC (permalink / raw) To: Simon Marchi, Tom Tromey; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] [gdb/tui] Factor out tui_noscroll_window et al 2024-02-22 12:07 ` Tom de Vries @ 2024-02-22 13:13 ` Tom de Vries 2024-02-22 15:07 ` Tom Tromey 0 siblings, 1 reply; 13+ messages in thread From: Tom de Vries @ 2024-02-22 13:13 UTC (permalink / raw) To: Simon Marchi, Tom Tromey; +Cc: gdb-patches 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 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] [gdb/tui] Factor out tui_noscroll_window et al 2024-02-22 13:13 ` Tom de Vries @ 2024-02-22 15:07 ` Tom Tromey 2024-02-22 15:26 ` Tom de Vries 0 siblings, 1 reply; 13+ messages in thread From: Tom Tromey @ 2024-02-22 15:07 UTC (permalink / raw) To: Tom de Vries; +Cc: Simon Marchi, Tom Tromey, gdb-patches >>>>> "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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] [gdb/tui] Factor out tui_noscroll_window et al 2024-02-22 15:07 ` Tom Tromey @ 2024-02-22 15:26 ` Tom de Vries 2024-02-23 1:39 ` Tom Tromey 0 siblings, 1 reply; 13+ messages in thread From: Tom de Vries @ 2024-02-22 15:26 UTC (permalink / raw) To: Tom Tromey; +Cc: Simon Marchi, gdb-patches 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] [gdb/tui] Factor out tui_noscroll_window et al 2024-02-22 15:26 ` Tom de Vries @ 2024-02-23 1:39 ` Tom Tromey 0 siblings, 0 replies; 13+ messages in thread From: Tom Tromey @ 2024-02-23 1:39 UTC (permalink / raw) To: Tom de Vries; +Cc: Tom Tromey, Simon Marchi, gdb-patches 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2024-02-23 1:39 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-11-12 15:06 [PATCH] [gdb/tui] Factor out tui_noscroll_window et al Tom de Vries 2023-11-13 9:37 ` Tom de Vries 2023-11-13 17:09 ` Tom Tromey 2023-11-14 15:02 ` Tom de Vries 2023-11-13 17:08 ` Tom Tromey 2023-11-13 21:03 ` Tom de Vries 2023-11-14 14:13 ` Tom Tromey 2024-02-21 17:32 ` Simon Marchi 2024-02-22 12:07 ` Tom de Vries 2024-02-22 13:13 ` Tom de Vries 2024-02-22 15:07 ` Tom Tromey 2024-02-22 15:26 ` Tom de Vries 2024-02-23 1:39 ` Tom Tromey
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).