[18/21] gdb: make interp_set a method of struct ui

  Move interp_set to ui::set_current_interpreter.

Change-Id: I7e83ca7732bb6229a1e61dc0e3832f54c20c3f52
---
 gdb/interps.c | 66 +++------------------------------------------------
 gdb/ui.c      | 51 +++++++++++++++++++++++++++++++++++++++
 gdb/ui.h      | 12 ++++++++++
 3 files changed, 66 insertions(+), 63 deletions(-)
  

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

> Move interp_set to ui::set_current_interpreter.
>
> Change-Id: I7e83ca7732bb6229a1e61dc0e3832f54c20c3f52
> ---
>  gdb/interps.c | 66 +++------------------------------------------------
>  gdb/ui.c      | 51 +++++++++++++++++++++++++++++++++++++++
>  gdb/ui.h      | 12 ++++++++++
>  3 files changed, 66 insertions(+), 63 deletions(-)
>
> diff --git a/gdb/interps.c b/gdb/interps.c
> index 18758f1f7af6..465c174ea794 100644
> --- a/gdb/interps.c
> +++ b/gdb/interps.c
> @@ -80,66 +80,6 @@ find_interp_factory (const char *name)
>    return nullptr;
>  }
>  
> -/* This sets the current interpreter to be INTERP.  If INTERP has not
> -   been initialized, then this will also run the init method.
> -
> -   The TOP_LEVEL parameter tells if this new interpreter is
> -   the top-level one.  The top-level is what is requested
> -   on the command line, and is responsible for reporting general
> -   notification about target state changes.  For example, if
> -   MI is the top-level interpreter, then it will always report
> -   events such as target stops and new thread creation, even if they
> -   are caused by CLI commands.  */
> -
> -static void
> -interp_set (ui *ui, struct interp *interp, bool top_level)
> -{
> -  struct interp *old_interp = ui->current_interpreter;
> -
> -  /* If we already have an interpreter, then trying to
> -     set top level interpreter is kinda pointless.  */
> -  gdb_assert (!top_level || !ui->current_interpreter);
> -  gdb_assert (!top_level || !ui->top_level_interpreter);
> -
> -  if (old_interp != NULL)
> -    {
> -      ui->m_current_uiout->flush ();
> -      old_interp->suspend ();
> -    }
> -
> -  ui->current_interpreter = interp;
> -  if (top_level)
> -    ui->top_level_interpreter = interp;
> -
> -  if (interpreter_p != interp->name ())
> -    interpreter_p = interp->name ();
> -
> -  bool warn_about_mi1 = false;
> -
> -  /* Run the init proc.  */
> -  if (!interp->inited)
> -    {
> -      interp->init (top_level);
> -      interp->inited = true;
> -
> -      if (streq (interp->name (), "mi1"))
> -	warn_about_mi1 = true;
> -    }
> -
> -  /* Do this only after the interpreter is initialized.  */
> -  ui->m_current_uiout = interp->interp_ui_out ();
> -
> -  /* Clear out any installed interpreter hooks/event handlers.  */
> -  clear_interpreter_hooks ();
> -
> -  interp->resume ();
> -
> -  if (warn_about_mi1)
> -    warning (_("MI version 1 is deprecated in GDB 13 and "
> -	       "will be removed in GDB 14.  Please upgrade "
> -	       "to a newer version of MI."));
> -}
> -
>  /* See interps.h.  */
>  
>  void
> @@ -151,7 +91,7 @@ set_top_level_interpreter (const char *name)
>    if (interp == NULL)
>      error (_("Interpreter `%s' unrecognized"), name);
>    /* Install it.  */
> -  interp_set (current_ui, interp, true);
> +  current_ui->set_current_interpreter (interp, true);
>  }
>  
>  void
> @@ -267,10 +207,10 @@ interpreter_exec_cmd (const char *args, int from_tty)
>    if (interp_to_use == NULL)
>      error (_("Could not find interpreter \"%s\"."), prules[0]);
>  
> -  interp_set (current_ui, interp_to_use, false);
> +  current_ui->set_current_interpreter (interp_to_use, false);
>    SCOPE_EXIT
>      {
> -      interp_set (current_ui, old_interp, false);
> +      current_ui->set_current_interpreter (old_interp, false);
>      };
>  
>    for (i = 1; i < nrules; i++)
> diff --git a/gdb/ui.c b/gdb/ui.c
> index 8c04dc92b89e..2db899eb9c31 100644
> --- a/gdb/ui.c
> +++ b/gdb/ui.c
> @@ -172,6 +172,57 @@ ui::lookup_interp (const char *name)
>  
>  /* See ui.h.  */
>  
> +void
> +ui::set_current_interpreter (interp *interp, bool top_level)
> +{
> +  struct interp *old_interp = this->current_interpreter;

Is there any way that we can assert something like:

  gdb_assert (this == interp->m_ui);

we maybe need an interp::mi() accessor function.

> +
> +  /* If we already have an interpreter, then trying to
> +     set top level interpreter is kinda pointless.  */
> +  gdb_assert (!top_level || !this->current_interpreter);
> +  gdb_assert (!top_level || !this->top_level_interpreter);
> +
> +  if (old_interp != NULL)
> +    {
> +      m_current_uiout->flush ();
> +      old_interp->suspend ();
> +    }
> +
> +  this->current_interpreter = interp;
> +  if (top_level)
> +    this->top_level_interpreter = interp;
> +
> +  if (interpreter_p != interp->name ())
> +    interpreter_p = interp->name ();
> +
> +  bool warn_about_mi1 = false;
> +
> +  /* Run the init proc.  */
> +  if (!interp->inited)
> +    {
> +      interp->init (top_level);
> +      interp->inited = true;
> +
> +      if (streq (interp->name (), "mi1"))
> +	warn_about_mi1 = true;
> +    }
> +
> +  /* Do this only after the interpreter is initialized.  */
> +  m_current_uiout = interp->interp_ui_out ();
> +
> +  /* Clear out any installed interpreter hooks/event handlers.  */
> +  clear_interpreter_hooks ();
> +
> +  interp->resume ();
> +
> +  if (warn_about_mi1)
> +    warning (_("MI version 1 is deprecated in GDB 13 and "
> +	       "will be removed in GDB 14.  Please upgrade "
> +	       "to a newer version of MI."));
> +}
> +
> +/* See ui.h.  */
> +
>  void
>  ui::unregister_file_handler ()
>  {
> diff --git a/gdb/ui.h b/gdb/ui.h
> index be89ab3d6848..4f6a32991d6d 100644
> --- a/gdb/ui.h
> +++ b/gdb/ui.h
> @@ -167,6 +167,18 @@ struct ui : public intrusive_list_node<ui>
>       interp_factory_register, return nullptr; otherwise return a pointer to
>       the interpreter.  */
>    interp *lookup_interp (const char *name);
> +
> +  /* This sets the current interpreter of this UI to be INTERP.  If INTERP has
> +     not been initialized, then this will also run the init method.
> +
> +     The TOP_LEVEL parameter tells if this new interpreter is

I know these aren't your words, but could we take this chance to update
this text to:

  The TOP_LEVEL parameter is true if this new interpreter is

which reads much nicer.

Thanks,
Andrew

> +     the top-level one.  The top-level is what is requested
> +     on the command line, and is responsible for reporting general
> +     notification about target state changes.  For example, if
> +     MI is the top-level interpreter, then it will always report
> +     events such as target stops and new thread creation, even if they
> +     are caused by CLI commands.  */
> +  void set_current_interpreter (interp *interp, bool top_level);
>  };
>  
>  /* The main UI.  This is the UI that is bound to stdin/stdout/stderr.
> -- 
> 2.42.0
  

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

Simon> +  if (warn_about_mi1)
Simon> +    warning (_("MI version 1 is deprecated in GDB 13 and "
Simon> +	       "will be removed in GDB 14.  Please upgrade "
Simon> +	       "to a newer version of MI."));

Supposedly we did this already... I guess I missed a spot.

Tom
  

>> diff --git a/gdb/ui.c b/gdb/ui.c
>> index 8c04dc92b89e..2db899eb9c31 100644
>> --- a/gdb/ui.c
>> +++ b/gdb/ui.c
>> @@ -172,6 +172,57 @@ ui::lookup_interp (const char *name)
>>  
>>  /* See ui.h.  */
>>  
>> +void
>> +ui::set_current_interpreter (interp *interp, bool top_level)
>> +{
>> +  struct interp *old_interp = this->current_interpreter;
> 
> Is there any way that we can assert something like:
> 
>   gdb_assert (this == interp->m_ui);
> 
> we maybe need an interp::mi() accessor function.
Ah lol, we had the same idea.  I'll add a patch on top of this one to
make this change, since I'd like to keep this one "just move stuff" as
much as possible.

>> diff --git a/gdb/ui.h b/gdb/ui.h
>> index be89ab3d6848..4f6a32991d6d 100644
>> --- a/gdb/ui.h
>> +++ b/gdb/ui.h
>> @@ -167,6 +167,18 @@ struct ui : public intrusive_list_node<ui>
>>       interp_factory_register, return nullptr; otherwise return a pointer to
>>       the interpreter.  */
>>    interp *lookup_interp (const char *name);
>> +
>> +  /* This sets the current interpreter of this UI to be INTERP.  If INTERP has
>> +     not been initialized, then this will also run the init method.
>> +
>> +     The TOP_LEVEL parameter tells if this new interpreter is
> 
> I know these aren't your words, but could we take this chance to update
> this text to:
> 
>   The TOP_LEVEL parameter is true if this new interpreter is
> 
> which reads much nicer.

I'll add a patch on top of this one to do this change.

Simon
  

On 9/12/23 09:41, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Simon> +  if (warn_about_mi1)
> Simon> +    warning (_("MI version 1 is deprecated in GDB 13 and "
> Simon> +	       "will be removed in GDB 14.  Please upgrade "
> Simon> +	       "to a newer version of MI."));
> 
> Supposedly we did this already... I guess I missed a spot.
> 
> Tom

I'll send a separate patch to remove this warning.

Simon