[RFA,4/4] Remove interp_name

diff mbox series

This removes the interp_name function.  It is only used a few spots --
one of which was only calling it on "this".  It's simpler to remove
it; and should class interp become opaque in the future, it will be
just as easy to update the two remaining spots to use an accessor.

ChangeLog
2018-04-29  Tom Tromey  <tom@tromey.com>

	* interps.c (interp_name): Remove.
	* mi/mi-interp.c (mi_interp::init): Update.
	* interps.h (interp_name): Remove.
	(~scoped_restore_interp): Update.
	* tui/tui.c (tui_enable): Update.
---
 gdb/ChangeLog      | 8 ++++++++
 gdb/interps.c      | 8 --------
 gdb/interps.h      | 4 +---
 gdb/mi/mi-interp.c | 2 --
 gdb/tui/tui.c      | 2 +-
 5 files changed, 10 insertions(+), 14 deletions(-)

-- 
2.13.6

On 04/30/2018 06:12 AM, Tom Tromey wrote:
> This removes the interp_name function.  It is only used a few spots --

> one of which was only calling it on "this".  It's simpler to remove

> it; and should class interp become opaque in the future, it will be

> just as easy to update the two remaining spots to use an accessor.


Yeah, IIRC, it used to be opaque until I C++ified it.  Not seeing
a need to go back.

> 

> ChangeLog

> 2018-04-29  Tom Tromey  <tom@tromey.com>

> 

> 	* interps.c (interp_name): Remove.

> 	* mi/mi-interp.c (mi_interp::init): Update.

> 	* interps.h (interp_name): Remove.

> 	(~scoped_restore_interp): Update.

> 	* tui/tui.c (tui_enable): Update.


> diff --git a/gdb/tui/tui.c b/gdb/tui/tui.c

> index 7943a61676..9e2520b4fc 100644

> --- a/gdb/tui/tui.c

> +++ b/gdb/tui/tui.c

> @@ -415,7 +415,7 @@ tui_enable (void)

>  

>        /* If the top level interpreter is not the console/tui (e.g.,

>  	 MI), enabling curses will certainly lose.  */

> -      interp = interp_name (top_level_interpreter ());

> +      interp = top_level_interpreter ()->name;

>        if (strcmp (interp, INTERP_TUI) != 0)

>  	error (_("Cannot enable the TUI when the interpreter is '%s'"), interp);


Not sure.  The name is supposedly read-only, which would
suggest renaming the field to m_name and adding a name()
getter.  WDYT?

Thanks,
Pedro Alves

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:


Pedro> Not sure.  The name is supposedly read-only, which would
Pedro> suggest renaming the field to m_name and adding a name()
Pedro> getter.  WDYT?

How about the appended as a follow-up patch to this series?

Tom

commit d525a99be1b02dda6c69007e31dd06f276378aea
Author: Tom Tromey <tom@tromey.com>
Date:   Fri May 25 12:39:51 2018 -0600

    Add "name" method to class interp
    
    In a review Pedro pointed out that interp::name is intended to be
    read-only, and so an accessor would be a better fit.  This patch
    renames the field and adds a "name" method that is used instead.
    
    ChangeLog
    2018-05-25  Tom Tromey  <tom@tromey.com>
    
            * tui/tui.c (tui_enable): Update.
            * mi/mi-interp.c (mi_interp::init): Update.
            * interps.h (class interp) <name>: New method.
            <m_name>: Rename from name.
            (~scoped_restore_interp): Update.
            * interps.c (interp::interp): Update.
            (interp_add, interp_set, interp_lookup_existing)
            (current_interp_named_p): Update.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 0bf350c255..a01f60c3b0 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,14 @@
+2018-05-25  Tom Tromey  <tom@tromey.com>
+
+	* tui/tui.c (tui_enable): Update.
+	* mi/mi-interp.c (mi_interp::init): Update.
+	* interps.h (class interp) <name>: New method.
+	<m_name>: Rename from name.
+	(~scoped_restore_interp): Update.
+	* interps.c (interp::interp): Update.
+	(interp_add, interp_set, interp_lookup_existing)
+	(current_interp_named_p): Update.
+
 2018-05-25  Tom Tromey	<tom@tromey.com>
 
 	* interps.c (interp_name): Remove.
diff --git a/gdb/interps.c b/gdb/interps.c
index 8ec9744fdf..789ae86946 100644
--- a/gdb/interps.c
+++ b/gdb/interps.c
@@ -78,8 +78,8 @@ static struct interp *interp_lookup_existing (struct ui *ui,
 					      const char *name);
 
 interp::interp (const char *name)
+  : m_name (xstrdup (name))
 {
-  this->name = xstrdup (name);
   this->inited = false;
 }
 
@@ -129,7 +129,7 @@ interp_add (struct ui *ui, struct interp *interp)
 {
   struct ui_interp_info *ui_interp = get_interp_info (ui);
 
-  gdb_assert (interp_lookup_existing (ui, interp->name) == NULL);
+  gdb_assert (interp_lookup_existing (ui, interp->name ()) == NULL);
 
   interp->next = ui_interp->interp_list;
   ui_interp->interp_list = interp;
@@ -170,11 +170,11 @@ interp_set (struct interp *interp, bool top_level)
   /* We use interpreter_p for the "set interpreter" variable, so we need
      to make sure we have a malloc'ed copy for the set command to free.  */
   if (interpreter_p != NULL
-      && strcmp (interp->name, interpreter_p) != 0)
+      && strcmp (interp->name (), interpreter_p) != 0)
     {
       xfree (interpreter_p);
 
-      interpreter_p = xstrdup (interp->name);
+      interpreter_p = xstrdup (interp->name ());
     }
 
   /* Run the init proc.  */
@@ -206,7 +206,7 @@ interp_lookup_existing (struct ui *ui, const char *name)
        interp != NULL;
        interp = interp->next)
     {
-      if (strcmp (interp->name, name) == 0)
+      if (strcmp (interp->name (), name) == 0)
 	return interp;
     }
 
@@ -282,7 +282,7 @@ current_interp_named_p (const char *interp_name)
   struct interp *interp = ui_interp->current_interpreter;
 
   if (interp != NULL)
-    return (strcmp (interp->name, interp_name) == 0);
+    return (strcmp (interp->name (), interp_name) == 0);
 
   return 0;
 }
diff --git a/gdb/interps.h b/gdb/interps.h
index a689be5625..74c9a80918 100644
--- a/gdb/interps.h
+++ b/gdb/interps.h
@@ -74,8 +74,13 @@ public:
   virtual bool supports_command_editing ()
   { return false; }
 
+  const char *name () const
+  {
+    return m_name;
+  }
+
   /* This is the name in "-i=" and "set interpreter".  */
-  const char *name;
+  const char *m_name;
 
   /* Interpreters are stored in a linked list, this is the next
      one...  */
@@ -111,7 +116,7 @@ public:
 
   ~scoped_restore_interp ()
   {
-    set_interp (m_interp->name);
+    set_interp (m_interp->name ());
   }
 
   scoped_restore_interp (const scoped_restore_interp &) = delete;
diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index e52d797973..ebc899f631 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -130,13 +130,13 @@ mi_interp::init (bool top_level)
 
   /* INTERP_MI selects the most recent released version.  "mi2" was
      released as part of GDB 6.0.  */
-  if (strcmp (name, INTERP_MI) == 0)
+  if (strcmp (name (), INTERP_MI) == 0)
     mi_version = 2;
-  else if (strcmp (name, INTERP_MI1) == 0)
+  else if (strcmp (name (), INTERP_MI1) == 0)
     mi_version = 1;
-  else if (strcmp (name, INTERP_MI2) == 0)
+  else if (strcmp (name (), INTERP_MI2) == 0)
     mi_version = 2;
-  else if (strcmp (name, INTERP_MI3) == 0)
+  else if (strcmp (name (), INTERP_MI3) == 0)
     mi_version = 3;
   else
     gdb_assert_not_reached ("unhandled MI version");
diff --git a/gdb/tui/tui.c b/gdb/tui/tui.c
index 9e2520b4fc..75a9ced619 100644
--- a/gdb/tui/tui.c
+++ b/gdb/tui/tui.c
@@ -415,7 +415,7 @@ tui_enable (void)
 
       /* If the top level interpreter is not the console/tui (e.g.,
 	 MI), enabling curses will certainly lose.  */
-      interp = top_level_interpreter ()->name;
+      interp = top_level_interpreter ()->name ();
       if (strcmp (interp, INTERP_TUI) != 0)
 	error (_("Cannot enable the TUI when the interpreter is '%s'"), interp);

On 05/25/2018 07:41 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

> 

> Pedro> Not sure.  The name is supposedly read-only, which would

> Pedro> suggest renaming the field to m_name and adding a name()

> Pedro> getter.  WDYT?

> 

> How about the appended as a follow-up patch to this series?


Thanks, looks good.

Pedro Alves