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

[Simon Marchi <simon.marchi@efficios.com>]

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