[PATCH 0/3] Some minor interps tweaks

[PATCH 0/3] Some minor interps tweaks

[Simon Marchi]

As I'm looking through interps.c doing a larger change, I came up with
these small cleanups.

Simon Marchi (3):
  gdb: make get_interp_info return a reference
  gdb: make interp::m_name an `const char *`
  gdb: initialize interp::next

 gdb/interps.c | 79 +++++++++++++++++++++++++--------------------------
 gdb/interps.h | 14 ++++-----
 2 files changed, 45 insertions(+), 48 deletions(-)

-- 
2.39.2

[PATCH 1/3] gdb: make get_interp_info return a reference

[Simon Marchi]

get_interp_info and get_current_interp_info always return non-nullptr,
so they can return a reference instead of a pointer.

Change-Id: I6d8dea92dc26a58ea340d04862db6b8d9cf906a0
---
 gdb/interps.c | 71 +++++++++++++++++++++++++--------------------------
 1 file changed, 35 insertions(+), 36 deletions(-)

diff --git a/gdb/interps.c b/gdb/interps.c
index 9c7908bde1ca..9517e57540d3 100644
--- a/gdb/interps.c
+++ b/gdb/interps.c
@@ -55,20 +55,19 @@ struct ui_interp_info
   struct interp *command_interpreter;
 };
 
-/* Get UI's ui_interp_info object.  Never returns NULL.  */
+/* Get UI's ui_interp_info object.  */
 
-static struct ui_interp_info *
+static ui_interp_info &
 get_interp_info (struct ui *ui)
 {
   if (ui->interp_info == NULL)
     ui->interp_info = XCNEW (struct ui_interp_info);
-  return ui->interp_info;
+  return *ui->interp_info;
 }
 
-/* Get the current UI's ui_interp_info object.  Never returns
-   NULL.  */
+/* Get the current UI's ui_interp_info object.  */
 
-static struct ui_interp_info *
+static ui_interp_info &
 get_current_interp_info (void)
 {
   return get_interp_info (current_ui);
@@ -128,12 +127,12 @@ interp_factory_register (const char *name, interp_factory_func func)
 static void
 interp_add (struct ui *ui, struct interp *interp)
 {
-  struct ui_interp_info *ui_interp = get_interp_info (ui);
+  ui_interp_info &ui_interp = get_interp_info (ui);
 
   gdb_assert (interp_lookup_existing (ui, interp->name ()) == NULL);
 
-  interp->next = ui_interp->interp_list;
-  ui_interp->interp_list = interp;
+  interp->next = ui_interp.interp_list;
+  ui_interp.interp_list = interp;
 }
 
 /* This sets the current interpreter to be INTERP.  If INTERP has not
@@ -150,13 +149,13 @@ interp_add (struct ui *ui, struct interp *interp)
 static void
 interp_set (struct interp *interp, bool top_level)
 {
-  struct ui_interp_info *ui_interp = get_current_interp_info ();
-  struct interp *old_interp = ui_interp->current_interpreter;
+  ui_interp_info &ui_interp = get_current_interp_info ();
+  struct interp *old_interp = ui_interp.current_interpreter;
 
   /* If we already have an interpreter, then trying to
      set top level interpreter is kinda pointless.  */
-  gdb_assert (!top_level || !ui_interp->current_interpreter);
-  gdb_assert (!top_level || !ui_interp->top_level_interpreter);
+  gdb_assert (!top_level || !ui_interp.current_interpreter);
+  gdb_assert (!top_level || !ui_interp.top_level_interpreter);
 
   if (old_interp != NULL)
     {
@@ -164,9 +163,9 @@ interp_set (struct interp *interp, bool top_level)
       old_interp->suspend ();
     }
 
-  ui_interp->current_interpreter = interp;
+  ui_interp.current_interpreter = interp;
   if (top_level)
-    ui_interp->top_level_interpreter = interp;
+    ui_interp.top_level_interpreter = interp;
 
   if (interpreter_p != interp->name ())
     interpreter_p = interp->name ();
@@ -203,10 +202,10 @@ interp_set (struct interp *interp, bool top_level)
 static struct interp *
 interp_lookup_existing (struct ui *ui, const char *name)
 {
-  struct ui_interp_info *ui_interp = get_interp_info (ui);
+  ui_interp_info &ui_interp = get_interp_info (ui);
   struct interp *interp;
 
-  for (interp = ui_interp->interp_list;
+  for (interp = ui_interp.interp_list;
        interp != NULL;
        interp = interp->next)
     {
@@ -259,8 +258,8 @@ void
 current_interp_set_logging (ui_file_up logfile, bool logging_redirect,
 			    bool debug_redirect)
 {
-  struct ui_interp_info *ui_interp = get_current_interp_info ();
-  struct interp *interp = ui_interp->current_interpreter;
+  ui_interp_info &ui_interp = get_current_interp_info ();
+  struct interp *interp = ui_interp.current_interpreter;
 
   interp->set_logging (std::move (logfile), logging_redirect, debug_redirect);
 }
@@ -269,12 +268,12 @@ current_interp_set_logging (ui_file_up logfile, bool logging_redirect,
 struct interp *
 scoped_restore_interp::set_interp (const char *name)
 {
-  struct ui_interp_info *ui_interp = get_current_interp_info ();
+  ui_interp_info &ui_interp = get_current_interp_info ();
   struct interp *interp = interp_lookup (current_ui, name);
-  struct interp *old_interp = ui_interp->current_interpreter;
+  struct interp *old_interp = ui_interp.current_interpreter;
 
   if (interp)
-    ui_interp->current_interpreter = interp;
+    ui_interp.current_interpreter = interp;
   return old_interp;
 }
 
@@ -282,8 +281,8 @@ scoped_restore_interp::set_interp (const char *name)
 int
 current_interp_named_p (const char *interp_name)
 {
-  struct ui_interp_info *ui_interp = get_current_interp_info ();
-  struct interp *interp = ui_interp->current_interpreter;
+  ui_interp_info &ui_interp = get_current_interp_info ();
+  struct interp *interp = ui_interp.current_interpreter;
 
   if (interp != NULL)
     return (strcmp (interp->name (), interp_name) == 0);
@@ -304,12 +303,12 @@ current_interp_named_p (const char *interp_name)
 struct interp *
 command_interp (void)
 {
-  struct ui_interp_info *ui_interp = get_current_interp_info ();
+  ui_interp_info &ui_interp = get_current_interp_info ();
 
-  if (ui_interp->command_interpreter != NULL)
-    return ui_interp->command_interpreter;
+  if (ui_interp.command_interpreter != NULL)
+    return ui_interp.command_interpreter;
   else
-    return ui_interp->current_interpreter;
+    return ui_interp.current_interpreter;
 }
 
 /* See interps.h.  */
@@ -336,11 +335,11 @@ interp_supports_command_editing (struct interp *interp)
 void
 interp_exec (struct interp *interp, const char *command_str)
 {
-  struct ui_interp_info *ui_interp = get_current_interp_info ();
+  ui_interp_info &ui_interp = get_current_interp_info ();
 
   /* See `command_interp' for why we do this.  */
   scoped_restore save_command_interp
-    = make_scoped_restore (&ui_interp->command_interpreter, interp);
+    = make_scoped_restore (&ui_interp.command_interpreter, interp);
 
   interp->exec (command_str);
 }
@@ -365,7 +364,7 @@ clear_interpreter_hooks (void)
 static void
 interpreter_exec_cmd (const char *args, int from_tty)
 {
-  struct ui_interp_info *ui_interp = get_current_interp_info ();
+  ui_interp_info &ui_interp = get_current_interp_info ();
   struct interp *old_interp, *interp_to_use;
   unsigned int nrules;
   unsigned int i;
@@ -387,7 +386,7 @@ interpreter_exec_cmd (const char *args, int from_tty)
   if (nrules < 2)
     error (_("Usage: interpreter-exec INTERPRETER COMMAND..."));
 
-  old_interp = ui_interp->current_interpreter;
+  old_interp = ui_interp.current_interpreter;
 
   interp_to_use = interp_lookup (current_ui, prules[0]);
   if (interp_to_use == NULL)
@@ -425,9 +424,9 @@ interpreter_completer (struct cmd_list_element *ignore,
 struct interp *
 top_level_interpreter (void)
 {
-  struct ui_interp_info *ui_interp = get_current_interp_info ();
+  ui_interp_info &ui_interp = get_current_interp_info ();
 
-  return ui_interp->top_level_interpreter;
+  return ui_interp.top_level_interpreter;
 }
 
 /* See interps.h.  */
@@ -435,9 +434,9 @@ top_level_interpreter (void)
 struct interp *
 current_interpreter (void)
 {
-  struct ui_interp_info *ui_interp = get_interp_info (current_ui);
+  ui_interp_info &ui_interp = get_interp_info (current_ui);
 
-  return ui_interp->current_interpreter;
+  return ui_interp.current_interpreter;
 }
 
 /* This just adds the "interpreter-exec" command.  */
-- 
2.39.2

[PATCH 2/3] gdb: make interp::m_name an `const char *`

[Simon Marchi]

From: Simon Marchi <simon.marchi@polymtl.ca>

I realized that the memory for interp names does not need to be
allocated.  The name used to register interp factory functions is always
a literal string, so has static storage duration.  If we change
interp_lookup to pass that name instead of the string that it receives
as a parameter (which does not always have static storage duration),
then interps can simply store pointers to the name.

So, change interp_lookup to pass `factory.name` rather than `name`.
Change interp::m_name to be a `const char *` rather than an std::string.

Change-Id: I0474d1f7b3512e7d172ccd73018aea927def3188
---
 gdb/interps.c |  8 +++-----
 gdb/interps.h | 12 ++++++------
 2 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/gdb/interps.c b/gdb/interps.c
index 9517e57540d3..5b964aebbbd6 100644
--- a/gdb/interps.c
+++ b/gdb/interps.c
@@ -79,13 +79,11 @@ static struct interp *interp_lookup_existing (struct ui *ui,
 					      const char *name);
 
 interp::interp (const char *name)
-  : m_name (make_unique_xstrdup (name))
+  : m_name (name)
 {
 }
 
-interp::~interp ()
-{
-}
+interp::~interp () = default;
 
 /* An interpreter factory.  Maps an interpreter name to the factory
    function that instantiates an interpreter by that name.  */
@@ -232,7 +230,7 @@ interp_lookup (struct ui *ui, const char *name)
   for (const interp_factory &factory : interpreter_factories)
     if (strcmp (factory.name, name) == 0)
       {
-	interp = factory.func (name);
+	interp = factory.func (factory.name);
 	interp_add (ui, interp);
 	return interp;
       }
diff --git a/gdb/interps.h b/gdb/interps.h
index 01bec47550dd..62f37951ddea 100644
--- a/gdb/interps.h
+++ b/gdb/interps.h
@@ -32,7 +32,9 @@ typedef struct interp *(*interp_factory_func) (const char *name);
 /* Each interpreter kind (CLI, MI, etc.) registers itself with a call
    to this function, passing along its name, and a pointer to a
    function that creates a new instance of an interpreter with that
-   name.  */
+   name.
+
+   The memory for NAME must have static storage duration.  */
 extern void interp_factory_register (const char *name,
 				     interp_factory_func func);
 
@@ -76,13 +78,11 @@ class interp
   { return false; }
 
   const char *name () const
-  {
-    return m_name.get ();
-  }
+  { return m_name; }
 
 private:
-  /* This is the name in "-i=" and "set interpreter".  */
-  gdb::unique_xmalloc_ptr<char> m_name;
+  /* The memory for this is static, it comes from literal strings (e.g. "cli").  */
+  const char *m_name;
 
 public:
   /* Interpreters are stored in a linked list, this is the next
-- 
2.39.2

[PATCH 3/3] gdb: initialize interp::next

[Simon Marchi]

From: Simon Marchi <simon.marchi@polymtl.ca>

This field is never initialized, it seems to me like it would be a good
idea to initialize it to nullptr to avoid bad surprises.

Change-Id: I8c04319d564f5d385d8bf0acee758f6ce28b4447
---
 gdb/interps.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/interps.h b/gdb/interps.h
index 62f37951ddea..29248093c671 100644
--- a/gdb/interps.h
+++ b/gdb/interps.h
@@ -87,7 +87,7 @@ class interp
 public:
   /* Interpreters are stored in a linked list, this is the next
      one...  */
-  struct interp *next;
+  interp *next = nullptr;
 
   /* Has the init method been run?  */
   bool inited = false;
-- 
2.39.2

Re: [PATCH 1/3] gdb: make get_interp_info return a reference

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> get_interp_info and get_current_interp_info always return non-nullptr,
Simon> so they can return a reference instead of a pointer.

Maybe it would be better here if ui_interp_info also disabled copying.
Otherwise it seems easy to accidentally drop a "&" and wind up modifying
a new copy.

Well, that's my first reaction anyway.  I'm not sure how much we should
really be worried about it.

Tom

Re: [PATCH 2/3] gdb: make interp::m_name an `const char *`

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> I realized that the memory for interp names does not need to be
Simon> allocated.  The name used to register interp factory functions is always
Simon> a literal string, so has static storage duration.  If we change
Simon> interp_lookup to pass that name instead of the string that it receives
Simon> as a parameter (which does not always have static storage duration),
Simon> then interps can simply store pointers to the name.

I wish we had a way to detect/enforce this.  That way lies rust I suppose.

Simon> So, change interp_lookup to pass `factory.name` rather than `name`.
Simon> Change interp::m_name to be a `const char *` rather than an std::string.

Reviewed-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCH 3/3] gdb: initialize interp::next

[Tom Tromey]

Simon> This field is never initialized, it seems to me like it would be a good
Simon> idea to initialize it to nullptr to avoid bad surprises.

Seems totally fine.
Reviewed-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCH 1/3] gdb: make get_interp_info return a reference

[Simon Marchi]

On 3/7/23 14:27, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Simon> get_interp_info and get_current_interp_info always return non-nullptr,
> Simon> so they can return a reference instead of a pointer.
> 
> Maybe it would be better here if ui_interp_info also disabled copying.
> Otherwise it seems easy to accidentally drop a "&" and wind up modifying
> a new copy.
> 
> Well, that's my first reaction anyway.  I'm not sure how much we should
> really be worried about it.

Well, it's the language we have to work with, it does what we ask it to
do.  Here, we don't need for ui_interp_info to be copyable, to we can do
it, but if we needed it to be copyable, it would be down to being
careful.

Here's what the patch looks like now:


From e446fa8ad63f2a33f6f7ffb2427d00998da3c781 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@efficios.com>
Date: Thu, 2 Mar 2023 15:32:22 -0500
Subject: [PATCH] gdb: make get_interp_info return a reference

get_interp_info and get_current_interp_info always return non-nullptr,
so they can return a reference instead of a pointer.

Since we don't need to copy it, make ui_interp_info non-copyiable, to
avoid a copying it in a local variable, instead of getting a reference.

Change-Id: I6d8dea92dc26a58ea340d04862db6b8d9cf906a0
---
 gdb/interps.c | 76 ++++++++++++++++++++++++++-------------------------
 1 file changed, 39 insertions(+), 37 deletions(-)

diff --git a/gdb/interps.c b/gdb/interps.c
index 9c7908bde1ca..9768f3cf8804 100644
--- a/gdb/interps.c
+++ b/gdb/interps.c
@@ -45,6 +45,9 @@
 
 struct ui_interp_info
 {
+  ui_interp_info () = default;
+  DISABLE_COPY_AND_ASSIGN(ui_interp_info);
+
   /* Each top level has its own independent set of interpreters.  */
   struct interp *interp_list;
   struct interp *current_interpreter;
@@ -55,20 +58,19 @@ struct ui_interp_info
   struct interp *command_interpreter;
 };
 
-/* Get UI's ui_interp_info object.  Never returns NULL.  */
+/* Get UI's ui_interp_info object.  */
 
-static struct ui_interp_info *
+static ui_interp_info &
 get_interp_info (struct ui *ui)
 {
   if (ui->interp_info == NULL)
-    ui->interp_info = XCNEW (struct ui_interp_info);
-  return ui->interp_info;
+    ui->interp_info = new ui_interp_info;
+  return *ui->interp_info;
 }
 
-/* Get the current UI's ui_interp_info object.  Never returns
-   NULL.  */
+/* Get the current UI's ui_interp_info object.  */
 
-static struct ui_interp_info *
+static ui_interp_info &
 get_current_interp_info (void)
 {
   return get_interp_info (current_ui);
@@ -128,12 +130,12 @@ interp_factory_register (const char *name, interp_factory_func func)
 static void
 interp_add (struct ui *ui, struct interp *interp)
 {
-  struct ui_interp_info *ui_interp = get_interp_info (ui);
+  ui_interp_info &ui_interp = get_interp_info (ui);
 
   gdb_assert (interp_lookup_existing (ui, interp->name ()) == NULL);
 
-  interp->next = ui_interp->interp_list;
-  ui_interp->interp_list = interp;
+  interp->next = ui_interp.interp_list;
+  ui_interp.interp_list = interp;
 }
 
 /* This sets the current interpreter to be INTERP.  If INTERP has not
@@ -150,13 +152,13 @@ interp_add (struct ui *ui, struct interp *interp)
 static void
 interp_set (struct interp *interp, bool top_level)
 {
-  struct ui_interp_info *ui_interp = get_current_interp_info ();
-  struct interp *old_interp = ui_interp->current_interpreter;
+  ui_interp_info &ui_interp = get_current_interp_info ();
+  struct interp *old_interp = ui_interp.current_interpreter;
 
   /* If we already have an interpreter, then trying to
      set top level interpreter is kinda pointless.  */
-  gdb_assert (!top_level || !ui_interp->current_interpreter);
-  gdb_assert (!top_level || !ui_interp->top_level_interpreter);
+  gdb_assert (!top_level || !ui_interp.current_interpreter);
+  gdb_assert (!top_level || !ui_interp.top_level_interpreter);
 
   if (old_interp != NULL)
     {
@@ -164,9 +166,9 @@ interp_set (struct interp *interp, bool top_level)
       old_interp->suspend ();
     }
 
-  ui_interp->current_interpreter = interp;
+  ui_interp.current_interpreter = interp;
   if (top_level)
-    ui_interp->top_level_interpreter = interp;
+    ui_interp.top_level_interpreter = interp;
 
   if (interpreter_p != interp->name ())
     interpreter_p = interp->name ();
@@ -203,10 +205,10 @@ interp_set (struct interp *interp, bool top_level)
 static struct interp *
 interp_lookup_existing (struct ui *ui, const char *name)
 {
-  struct ui_interp_info *ui_interp = get_interp_info (ui);
+  ui_interp_info &ui_interp = get_interp_info (ui);
   struct interp *interp;
 
-  for (interp = ui_interp->interp_list;
+  for (interp = ui_interp.interp_list;
        interp != NULL;
        interp = interp->next)
     {
@@ -259,8 +261,8 @@ void
 current_interp_set_logging (ui_file_up logfile, bool logging_redirect,
 			    bool debug_redirect)
 {
-  struct ui_interp_info *ui_interp = get_current_interp_info ();
-  struct interp *interp = ui_interp->current_interpreter;
+  ui_interp_info &ui_interp = get_current_interp_info ();
+  struct interp *interp = ui_interp.current_interpreter;
 
   interp->set_logging (std::move (logfile), logging_redirect, debug_redirect);
 }
@@ -269,12 +271,12 @@ current_interp_set_logging (ui_file_up logfile, bool logging_redirect,
 struct interp *
 scoped_restore_interp::set_interp (const char *name)
 {
-  struct ui_interp_info *ui_interp = get_current_interp_info ();
+  ui_interp_info &ui_interp = get_current_interp_info ();
   struct interp *interp = interp_lookup (current_ui, name);
-  struct interp *old_interp = ui_interp->current_interpreter;
+  struct interp *old_interp = ui_interp.current_interpreter;
 
   if (interp)
-    ui_interp->current_interpreter = interp;
+    ui_interp.current_interpreter = interp;
   return old_interp;
 }
 
@@ -282,8 +284,8 @@ scoped_restore_interp::set_interp (const char *name)
 int
 current_interp_named_p (const char *interp_name)
 {
-  struct ui_interp_info *ui_interp = get_current_interp_info ();
-  struct interp *interp = ui_interp->current_interpreter;
+  ui_interp_info &ui_interp = get_current_interp_info ();
+  struct interp *interp = ui_interp.current_interpreter;
 
   if (interp != NULL)
     return (strcmp (interp->name (), interp_name) == 0);
@@ -304,12 +306,12 @@ current_interp_named_p (const char *interp_name)
 struct interp *
 command_interp (void)
 {
-  struct ui_interp_info *ui_interp = get_current_interp_info ();
+  ui_interp_info &ui_interp = get_current_interp_info ();
 
-  if (ui_interp->command_interpreter != NULL)
-    return ui_interp->command_interpreter;
+  if (ui_interp.command_interpreter != NULL)
+    return ui_interp.command_interpreter;
   else
-    return ui_interp->current_interpreter;
+    return ui_interp.current_interpreter;
 }
 
 /* See interps.h.  */
@@ -336,11 +338,11 @@ interp_supports_command_editing (struct interp *interp)
 void
 interp_exec (struct interp *interp, const char *command_str)
 {
-  struct ui_interp_info *ui_interp = get_current_interp_info ();
+  ui_interp_info &ui_interp = get_current_interp_info ();
 
   /* See `command_interp' for why we do this.  */
   scoped_restore save_command_interp
-    = make_scoped_restore (&ui_interp->command_interpreter, interp);
+    = make_scoped_restore (&ui_interp.command_interpreter, interp);
 
   interp->exec (command_str);
 }
@@ -365,7 +367,7 @@ clear_interpreter_hooks (void)
 static void
 interpreter_exec_cmd (const char *args, int from_tty)
 {
-  struct ui_interp_info *ui_interp = get_current_interp_info ();
+  ui_interp_info &ui_interp = get_current_interp_info ();
   struct interp *old_interp, *interp_to_use;
   unsigned int nrules;
   unsigned int i;
@@ -387,7 +389,7 @@ interpreter_exec_cmd (const char *args, int from_tty)
   if (nrules < 2)
     error (_("Usage: interpreter-exec INTERPRETER COMMAND..."));
 
-  old_interp = ui_interp->current_interpreter;
+  old_interp = ui_interp.current_interpreter;
 
   interp_to_use = interp_lookup (current_ui, prules[0]);
   if (interp_to_use == NULL)
@@ -425,9 +427,9 @@ interpreter_completer (struct cmd_list_element *ignore,
 struct interp *
 top_level_interpreter (void)
 {
-  struct ui_interp_info *ui_interp = get_current_interp_info ();
+  ui_interp_info &ui_interp = get_current_interp_info ();
 
-  return ui_interp->top_level_interpreter;
+  return ui_interp.top_level_interpreter;
 }
 
 /* See interps.h.  */
@@ -435,9 +437,9 @@ top_level_interpreter (void)
 struct interp *
 current_interpreter (void)
 {
-  struct ui_interp_info *ui_interp = get_interp_info (current_ui);
+  ui_interp_info &ui_interp = get_interp_info (current_ui);
 
-  return ui_interp->current_interpreter;
+  return ui_interp.current_interpreter;
 }
 
 /* This just adds the "interpreter-exec" command.  */

base-commit: 4779ed9757fa71e6743fb7fc1f9eeae8267ae36c
-- 
2.39.2

Re: [PATCH 1/3] gdb: make get_interp_info return a reference

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> Here's what the patch looks like now:

Simon>    if (ui->interp_info == NULL)
Simon> -    ui->interp_info = XCNEW (struct ui_interp_info);
Simon> -  return ui->interp_info;
Simon> +    ui->interp_info = new ui_interp_info;

Unfortunately XCNEW -> new also means adding member initializers to
ensure they are 0.

Tom

Re: [PATCH 1/3] gdb: make get_interp_info return a reference

[Simon Marchi]

On 3/7/23 15:45, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Simon> Here's what the patch looks like now:
> 
> Simon>    if (ui->interp_info == NULL)
> Simon> -    ui->interp_info = XCNEW (struct ui_interp_info);
> Simon> -  return ui->interp_info;
> Simon> +    ui->interp_info = new ui_interp_info;
> 
> Unfortunately XCNEW -> new also means adding member initializers to
> ensure they are 0.

Didn't I?  Oh, I did but it's uncommitted in my git tree.  I swear!

Here's the updated patch.


From 67119cc8ca5e2d4729f39455829fdc5fa095932a Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@efficios.com>
Date: Thu, 2 Mar 2023 15:32:22 -0500
Subject: [PATCH] gdb: make get_interp_info return a reference

get_interp_info and get_current_interp_info always return non-nullptr,
so they can return a reference instead of a pointer.

Since we don't need to copy it, make ui_interp_info non-copyiable, to
avoid a copying it in a local variable, instead of getting a reference.

Change-Id: I6d8dea92dc26a58ea340d04862db6b8d9cf906a0
---
 gdb/interps.c | 84 ++++++++++++++++++++++++++-------------------------
 1 file changed, 43 insertions(+), 41 deletions(-)

diff --git a/gdb/interps.c b/gdb/interps.c
index 9c7908bde1ca..12a0048d1020 100644
--- a/gdb/interps.c
+++ b/gdb/interps.c
@@ -45,30 +45,32 @@
 
 struct ui_interp_info
 {
+  ui_interp_info () = default;
+  DISABLE_COPY_AND_ASSIGN (ui_interp_info);
+
   /* Each top level has its own independent set of interpreters.  */
-  struct interp *interp_list;
-  struct interp *current_interpreter;
-  struct interp *top_level_interpreter;
+  interp *interp_list = nullptr;
+  interp *current_interpreter = nullptr;
+  interp *top_level_interpreter = nullptr;
 
   /* The interpreter that is active while `interp_exec' is active, NULL
      at all other times.  */
-  struct interp *command_interpreter;
+  interp *command_interpreter = nullptr;
 };
 
-/* Get UI's ui_interp_info object.  Never returns NULL.  */
+/* Get UI's ui_interp_info object.  */
 
-static struct ui_interp_info *
+static ui_interp_info &
 get_interp_info (struct ui *ui)
 {
   if (ui->interp_info == NULL)
-    ui->interp_info = XCNEW (struct ui_interp_info);
-  return ui->interp_info;
+    ui->interp_info = new ui_interp_info;
+  return *ui->interp_info;
 }
 
-/* Get the current UI's ui_interp_info object.  Never returns
-   NULL.  */
+/* Get the current UI's ui_interp_info object.  */
 
-static struct ui_interp_info *
+static ui_interp_info &
 get_current_interp_info (void)
 {
   return get_interp_info (current_ui);
@@ -128,12 +130,12 @@ interp_factory_register (const char *name, interp_factory_func func)
 static void
 interp_add (struct ui *ui, struct interp *interp)
 {
-  struct ui_interp_info *ui_interp = get_interp_info (ui);
+  ui_interp_info &ui_interp = get_interp_info (ui);
 
   gdb_assert (interp_lookup_existing (ui, interp->name ()) == NULL);
 
-  interp->next = ui_interp->interp_list;
-  ui_interp->interp_list = interp;
+  interp->next = ui_interp.interp_list;
+  ui_interp.interp_list = interp;
 }
 
 /* This sets the current interpreter to be INTERP.  If INTERP has not
@@ -150,13 +152,13 @@ interp_add (struct ui *ui, struct interp *interp)
 static void
 interp_set (struct interp *interp, bool top_level)
 {
-  struct ui_interp_info *ui_interp = get_current_interp_info ();
-  struct interp *old_interp = ui_interp->current_interpreter;
+  ui_interp_info &ui_interp = get_current_interp_info ();
+  struct interp *old_interp = ui_interp.current_interpreter;
 
   /* If we already have an interpreter, then trying to
      set top level interpreter is kinda pointless.  */
-  gdb_assert (!top_level || !ui_interp->current_interpreter);
-  gdb_assert (!top_level || !ui_interp->top_level_interpreter);
+  gdb_assert (!top_level || !ui_interp.current_interpreter);
+  gdb_assert (!top_level || !ui_interp.top_level_interpreter);
 
   if (old_interp != NULL)
     {
@@ -164,9 +166,9 @@ interp_set (struct interp *interp, bool top_level)
       old_interp->suspend ();
     }
 
-  ui_interp->current_interpreter = interp;
+  ui_interp.current_interpreter = interp;
   if (top_level)
-    ui_interp->top_level_interpreter = interp;
+    ui_interp.top_level_interpreter = interp;
 
   if (interpreter_p != interp->name ())
     interpreter_p = interp->name ();
@@ -203,10 +205,10 @@ interp_set (struct interp *interp, bool top_level)
 static struct interp *
 interp_lookup_existing (struct ui *ui, const char *name)
 {
-  struct ui_interp_info *ui_interp = get_interp_info (ui);
+  ui_interp_info &ui_interp = get_interp_info (ui);
   struct interp *interp;
 
-  for (interp = ui_interp->interp_list;
+  for (interp = ui_interp.interp_list;
        interp != NULL;
        interp = interp->next)
     {
@@ -259,8 +261,8 @@ void
 current_interp_set_logging (ui_file_up logfile, bool logging_redirect,
 			    bool debug_redirect)
 {
-  struct ui_interp_info *ui_interp = get_current_interp_info ();
-  struct interp *interp = ui_interp->current_interpreter;
+  ui_interp_info &ui_interp = get_current_interp_info ();
+  struct interp *interp = ui_interp.current_interpreter;
 
   interp->set_logging (std::move (logfile), logging_redirect, debug_redirect);
 }
@@ -269,12 +271,12 @@ current_interp_set_logging (ui_file_up logfile, bool logging_redirect,
 struct interp *
 scoped_restore_interp::set_interp (const char *name)
 {
-  struct ui_interp_info *ui_interp = get_current_interp_info ();
+  ui_interp_info &ui_interp = get_current_interp_info ();
   struct interp *interp = interp_lookup (current_ui, name);
-  struct interp *old_interp = ui_interp->current_interpreter;
+  struct interp *old_interp = ui_interp.current_interpreter;
 
   if (interp)
-    ui_interp->current_interpreter = interp;
+    ui_interp.current_interpreter = interp;
   return old_interp;
 }
 
@@ -282,8 +284,8 @@ scoped_restore_interp::set_interp (const char *name)
 int
 current_interp_named_p (const char *interp_name)
 {
-  struct ui_interp_info *ui_interp = get_current_interp_info ();
-  struct interp *interp = ui_interp->current_interpreter;
+  ui_interp_info &ui_interp = get_current_interp_info ();
+  struct interp *interp = ui_interp.current_interpreter;
 
   if (interp != NULL)
     return (strcmp (interp->name (), interp_name) == 0);
@@ -304,12 +306,12 @@ current_interp_named_p (const char *interp_name)
 struct interp *
 command_interp (void)
 {
-  struct ui_interp_info *ui_interp = get_current_interp_info ();
+  ui_interp_info &ui_interp = get_current_interp_info ();
 
-  if (ui_interp->command_interpreter != NULL)
-    return ui_interp->command_interpreter;
+  if (ui_interp.command_interpreter != NULL)
+    return ui_interp.command_interpreter;
   else
-    return ui_interp->current_interpreter;
+    return ui_interp.current_interpreter;
 }
 
 /* See interps.h.  */
@@ -336,11 +338,11 @@ interp_supports_command_editing (struct interp *interp)
 void
 interp_exec (struct interp *interp, const char *command_str)
 {
-  struct ui_interp_info *ui_interp = get_current_interp_info ();
+  ui_interp_info &ui_interp = get_current_interp_info ();
 
   /* See `command_interp' for why we do this.  */
   scoped_restore save_command_interp
-    = make_scoped_restore (&ui_interp->command_interpreter, interp);
+    = make_scoped_restore (&ui_interp.command_interpreter, interp);
 
   interp->exec (command_str);
 }
@@ -365,7 +367,7 @@ clear_interpreter_hooks (void)
 static void
 interpreter_exec_cmd (const char *args, int from_tty)
 {
-  struct ui_interp_info *ui_interp = get_current_interp_info ();
+  ui_interp_info &ui_interp = get_current_interp_info ();
   struct interp *old_interp, *interp_to_use;
   unsigned int nrules;
   unsigned int i;
@@ -387,7 +389,7 @@ interpreter_exec_cmd (const char *args, int from_tty)
   if (nrules < 2)
     error (_("Usage: interpreter-exec INTERPRETER COMMAND..."));
 
-  old_interp = ui_interp->current_interpreter;
+  old_interp = ui_interp.current_interpreter;
 
   interp_to_use = interp_lookup (current_ui, prules[0]);
   if (interp_to_use == NULL)
@@ -425,9 +427,9 @@ interpreter_completer (struct cmd_list_element *ignore,
 struct interp *
 top_level_interpreter (void)
 {
-  struct ui_interp_info *ui_interp = get_current_interp_info ();
+  ui_interp_info &ui_interp = get_current_interp_info ();
 
-  return ui_interp->top_level_interpreter;
+  return ui_interp.top_level_interpreter;
 }
 
 /* See interps.h.  */
@@ -435,9 +437,9 @@ top_level_interpreter (void)
 struct interp *
 current_interpreter (void)
 {
-  struct ui_interp_info *ui_interp = get_interp_info (current_ui);
+  ui_interp_info &ui_interp = get_interp_info (current_ui);
 
-  return ui_interp->current_interpreter;
+  return ui_interp.current_interpreter;
 }
 
 /* This just adds the "interpreter-exec" command.  */

base-commit: 4779ed9757fa71e6743fb7fc1f9eeae8267ae36c
-- 
2.39.2

Re: [PATCH 1/3] gdb: make get_interp_info return a reference

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> Didn't I?  Oh, I did but it's uncommitted in my git tree.  I swear!

Been there myself.

Simon> Here's the updated patch.

Thanks.
Reviewed-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCH 1/3] gdb: make get_interp_info return a reference

[Simon Marchi]

On 3/7/23 16:29, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Simon> Didn't I?  Oh, I did but it's uncommitted in my git tree.  I swear!
> 
> Been there myself.
> 
> Simon> Here's the updated patch.
> 
> Thanks.
> Reviewed-By: Tom Tromey <tom@tromey.com>
> 
> Tom

Thanks, pushed the whole series.

Simon