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

[Simon Marchi <simark@simark.ca>]

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