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

[Simon Marchi <simark@simark.ca>]

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