Re: [PATCH 04/21] gdb: make interp_lookup a method of struct ui

[Andrew Burgess <aburgess@redhat.com>]

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

> This requires exposing some things about interpreter factories in
> interps.h, for ui.c to use it.  No behavior changes expected.
>
> Change-Id: I7a9e93621c0588e367b5356916d4dad90757bb3d
> ---
>  gdb/cli/cli-script.c |  2 +-
>  gdb/interps.c        | 58 ++++++++++++--------------------------------
>  gdb/interps.h        | 25 ++++++++++++++-----
>  gdb/mi/mi-interp.c   |  4 +--
>  gdb/python/python.c  |  2 +-
>  gdb/ui.c             | 25 ++++++++++++++++++-
>  gdb/ui.h             |  6 +++++
>  7 files changed, 68 insertions(+), 54 deletions(-)
>
> diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c
> index 8ec5689ebcfd..49ba854c2a09 100644
> --- a/gdb/cli/cli-script.c
> +++ b/gdb/cli/cli-script.c
> @@ -702,7 +702,7 @@ execute_control_command (struct command_line *cmd, int from_tty)
>  
>    /* Make sure we use the console uiout.  It's possible that we are executing
>       breakpoint commands while running the MI interpreter.  */
> -  interp *console = interp_lookup (current_ui, INTERP_CONSOLE);
> +  interp *console = current_ui->lookup_interp (INTERP_CONSOLE);
>    scoped_restore save_uiout
>      = make_scoped_restore (&current_uiout, console->interp_ui_out ());
>  
> diff --git a/gdb/interps.c b/gdb/interps.c
> index b05a6c4eb875..bd95c3175adb 100644
> --- a/gdb/interps.c
> +++ b/gdb/interps.c
> @@ -48,22 +48,6 @@ interp::interp (const char *name)
>  
>  interp::~interp () = default;
>  
> -/* An interpreter factory.  Maps an interpreter name to the factory
> -   function that instantiates an interpreter by that name.  */
> -
> -struct interp_factory
> -{
> -  interp_factory (const char *name_, interp_factory_func func_)
> -  : name (name_), func (func_)
> -  {}
> -
> -  /* This is the name in "-i=INTERP" and "interpreter-exec INTERP".  */
> -  const char *name;
> -
> -  /* The function that creates the interpreter.  */
> -  interp_factory_func func;
> -};
> -
>  /* The registered interpreter factories.  */
>  static std::vector<interp_factory> interpreter_factories;
>  
> @@ -83,6 +67,18 @@ interp_factory_register (const char *name, interp_factory_func func)
>    interpreter_factories.emplace_back (name, func);
>  }
>  
> +/* See interps.h.  */
> +
> +const interp_factory *
> +find_interp_factory (const char *name)
> +{
> +  for (const interp_factory &factory : interpreter_factories)
> +    if (strcmp (factory.name, name) == 0)
> +      return &factory;
> +
> +  return nullptr;
> +}
> +
>  /* This sets the current interpreter to be INTERP.  If INTERP has not
>     been initialized, then this will also run the init method.
>  
> @@ -145,35 +141,11 @@ interp_set (struct interp *interp, bool top_level)
>  
>  /* See interps.h.  */
>  
> -struct interp *
> -interp_lookup (struct ui *ui, const char *name)
> -{
> -  if (name == NULL || strlen (name) == 0)
> -    return NULL;
> -
> -  /* Only create each interpreter once per top level.  */
> -  interp *interp = ui->lookup_existing_interp (name);
> -  if (interp != NULL)
> -    return interp;
> -
> -  for (const interp_factory &factory : interpreter_factories)
> -    if (strcmp (factory.name, name) == 0)
> -      {
> -	interp = factory.func (factory.name);
> -	ui->add_interp (interp);
> -	return interp;
> -      }
> -
> -  return NULL;
> -}
> -
> -/* See interps.h.  */
> -
>  void
>  set_top_level_interpreter (const char *name)
>  {
>    /* Find it.  */
> -  struct interp *interp = interp_lookup (current_ui, name);
> +  struct interp *interp = current_ui->lookup_interp (name);
>  
>    if (interp == NULL)
>      error (_("Interpreter `%s' unrecognized"), name);
> @@ -194,7 +166,7 @@ current_interp_set_logging (ui_file_up logfile, bool logging_redirect,
>  struct interp *
>  scoped_restore_interp::set_interp (const char *name)
>  {
> -  struct interp *interp = interp_lookup (current_ui, name);
> +  struct interp *interp = current_ui->lookup_interp (name);
>    struct interp *old_interp = current_ui->current_interpreter;
>  
>    if (interp)
> @@ -290,7 +262,7 @@ interpreter_exec_cmd (const char *args, int from_tty)
>  
>    interp *old_interp = current_ui->current_interpreter;
>  
> -  interp_to_use = interp_lookup (current_ui, prules[0]);
> +  interp_to_use = current_ui->lookup_interp (prules[0]);
>    if (interp_to_use == NULL)
>      error (_("Could not find interpreter \"%s\"."), prules[0]);
>  
> diff --git a/gdb/interps.h b/gdb/interps.h
> index 287df2c8c810..433d92439eba 100644
> --- a/gdb/interps.h
> +++ b/gdb/interps.h
> @@ -36,6 +36,22 @@ struct trace_state_variable;
>  
>  typedef struct interp *(*interp_factory_func) (const char *name);
>  
> +/* An interpreter factory.  Maps an interpreter name to the factory
> +   function that instantiates an interpreter by that name.  */
> +
> +struct interp_factory
> +{
> +  interp_factory (const char *name_, interp_factory_func func_)
> +  : name (name_), func (func_)
> +  {}
> +
> +  /* This is the name in "-i=INTERP" and "interpreter-exec INTERP".  */
> +  const char *name;
> +
> +  /* The function that creates the interpreter.  */
> +  interp_factory_func func;
> +};
> +
>  /* 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
> @@ -45,6 +61,9 @@ typedef struct interp *(*interp_factory_func) (const char *name);
>  extern void interp_factory_register (const char *name,
>  				     interp_factory_func func);
>  
> +/* Return the interpreter factory for NAME.  Return nullptr if not found.  */
> +extern const interp_factory *find_interp_factory (const char *name);
> +
>  extern void interp_exec (struct interp *interp, const char *command);
>  
>  class interp : public intrusive_list_node<interp>
> @@ -193,12 +212,6 @@ class interp : public intrusive_list_node<interp>
>    bool inited = false;
>  };
>  
> -/* Look up the interpreter for NAME, creating one if none exists yet.
> -   If NAME is not a interpreter type previously registered with
> -   interp_factory_register, return NULL; otherwise return a pointer to
> -   the interpreter.  */
> -extern struct interp *interp_lookup (struct ui *ui, const char *name);
> -
>  /* Set the current UI's top level interpreter to the interpreter named
>     NAME.  Throws an error if NAME is not a known interpreter or the
>     interpreter fails to initialize.  */
> diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
> index b3b0f5bb1f51..946fed5867c4 100644
> --- a/gdb/mi/mi-interp.c
> +++ b/gdb/mi/mi-interp.c
> @@ -162,7 +162,7 @@ mi_cmd_interpreter_exec (const char *command, const char *const *argv,
>      error (_("-interpreter-exec: "
>  	     "Usage: -interpreter-exec interp command"));
>  
> -  interp_to_use = interp_lookup (current_ui, argv[0]);
> +  interp_to_use = current_ui->lookup_interp (argv[0]);
>    if (interp_to_use == NULL)
>      error (_("-interpreter-exec: could not find interpreter \"%s\""),
>  	   argv[0]);
> @@ -416,7 +416,7 @@ mi_interp::on_normal_stop (struct bpstat *bs, int print_frame)
>  	  mi_uiout->field_string ("reason", async_reason_lookup (reason));
>  	}
>  
> -      interp *console_interp = interp_lookup (current_ui, INTERP_CONSOLE);
> +      interp *console_interp = current_ui->lookup_interp (INTERP_CONSOLE);
>  
>        /* We only want to print the displays once, and we want it to
>  	 look just how it would on the console, so we use this to
> diff --git a/gdb/python/python.c b/gdb/python/python.c
> index 6a978d632e9a..a1aaa29d0ce5 100644
> --- a/gdb/python/python.c
> +++ b/gdb/python/python.c
> @@ -683,7 +683,7 @@ execute_gdb_command (PyObject *self, PyObject *args, PyObject *kw)
>  
>  	/* Use the console interpreter uiout to have the same print format
>  	   for console or MI.  */
> -	interp = interp_lookup (current_ui, "console");
> +	interp = current_ui->lookup_interp ("console");

Might as well take this opportunity to replace "console" with
INTERP_CONSOLE which is what we use everywhere else.

>  	current_uiout = interp->interp_ui_out ();
>  
>  	if (to_string)
> diff --git a/gdb/ui.c b/gdb/ui.c
> index 624187ac73c3..a7b81c077e9a 100644
> --- a/gdb/ui.c
> +++ b/gdb/ui.c
> @@ -169,7 +169,30 @@ ui::add_interp (interp *interp)
>    this->interp_list.push_back (*interp);
>  }
>  
> -/* See top.h.  */
> +/* See interps.h.  */
> +
> +interp *
> +ui::lookup_interp (const char *name)
> +{
> +  if (name == nullptr || strlen (name) == 0)
> +    return nullptr;
> +
> +  /* Only create each interpreter once per UI.  */
> +  interp *interp = this->lookup_existing_interp (name);
> +  if (interp != nullptr)
> +    return interp;
> +
> +  const interp_factory *factory = find_interp_factory (name);
> +  if (factory == nullptr)
> +    return nullptr;
> +
> +  interp = factory->func (factory->name);
> +  this->add_interp (interp);
> +
> +  return interp;
> +}

Rather than exposing the interpreter factor stuff outside of interp.c, I
think a better approach would be either move ui::lookup_interp into
interp.c -- personally, I'm happy to see different parts of an object in
different .c files when that makes sense.

But if you prefer all to keep ui implementation in ui.c, rather than
calling find_interp_factory and then factory->func, we could just have a
new function `create_interp (name)` which creates an interpreter, or
returns nullptr, then we can write:

  interp = create_interp (name);
  if (interp != nullptr)
    this->add_interp (interp);
  return interp;

And all the factory stuff can remain private to interp.c.

Thoughts?

Thanks,
Andrew

> +
> +/* See ui.h.  */
>  
>  void
>  ui::unregister_file_handler ()
> diff --git a/gdb/ui.h b/gdb/ui.h
> index 48cad96b3fb8..aeb26c68823a 100644
> --- a/gdb/ui.h
> +++ b/gdb/ui.h
> @@ -163,6 +163,12 @@ struct ui : public intrusive_list_node<ui>
>       return nullptr, otherwise return a pointer to the interpreter.  */
>    interp *lookup_existing_interp (const char *name) const;
>  
> +  /* Look up the interpreter for NAME, creating one if none exists yet.
> +     If NAME is not a interpreter type previously registered with
> +     interp_factory_register, return nullptr; otherwise return a pointer to
> +     the interpreter.  */
> +  interp *lookup_interp (const char *name);
> +
>    /* Add interpreter INTERP to this UI's interpreter list.  The
>       interpreter must not have previously been added.  */
>    void add_interp (interp *interp);
> -- 
> 2.42.0