[2/3] Add parameter_changed observer

  This adds a parameter_changed observer and has it fire in
do_set_command.  The interpreter code is updated to use this
mechanism.
---
 gdb/cli/cli-setshow.c | 14 +++++++-------
 gdb/interps.c         |  4 ++++
 gdb/observable.c      |  1 +
 gdb/observable.h      |  7 +++++++
 4 files changed, 19 insertions(+), 7 deletions(-)
  

Tom Tromey <tom@tromey.com> writes:

> This adds a parameter_changed observer and has it fire in
> do_set_command.  The interpreter code is updated to use this
> mechanism.

Is there a reason why the TUI interpreter couldn't just be wired up to
listen for the ::param_changed interpreter call?  In general the TUI
seems to be using observers over the interps_notify_* function API.  I
wonder why that is?

I'm OK with adding an observer here though, as we might want to expose
this in the Python API (later on, not required in this patch), as user
extensions might want to respond to setting changes too.

However, I think we should follow the model of, for example,
notify_breakpoint_deleted, where we have a single function which calls
the interps_notify_param_changed function, followed by triggering the
observers.

I guess in this case it likely doesn't matter, but the existing code
(for e.g. breakpoints) is the way it is because we want interpreters to
see the events before other general observers (which is why I'm curious
that TUI uses observers, not the interps method API).  Still, if we
start doing things differently in this case there's a risk that someone
will copy this model later on for a case where the ordering really does
matter.

So, TLDR, I'm asking that you add and use this function:

  void
  notify_param_changed (const char *param, const char *value)
  {
    interps_notify_param_changed (param, value);
    gdb::observers::param_changed.notify (param, value);
  }

Then, I think you should make use of this function in source.c too,
replacing the existing current interps_notify_param_changed call.  When
you add the TUI changes in the next patch, the TUI is currently missing
changes to the source directories list.

This is important because, if you build an application then move the
source code, start GDB, and do 'layout src', you'll get the "No Source"
message.  If you then 'directory /path/to/source/' it would be nice if
the source window updated.  This doesn't happen right now (after patch
#3). 

Hope that all make sense.

Thanks,
Andrew




> ---
>  gdb/cli/cli-setshow.c | 14 +++++++-------
>  gdb/interps.c         |  4 ++++
>  gdb/observable.c      |  1 +
>  gdb/observable.h      |  7 +++++++
>  4 files changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/gdb/cli/cli-setshow.c b/gdb/cli/cli-setshow.c
> index f2199a145135c5f2b8b26c27dbcc38e36f0c28ac..b26c504fd14f45b9f9c16032b68d47b1cfbf5809 100644
> --- a/gdb/cli/cli-setshow.c
> +++ b/gdb/cli/cli-setshow.c
> @@ -486,18 +486,18 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
>  	case var_string_noescape:
>  	case var_filename:
>  	case var_optional_filename:
> -	  interps_notify_param_changed
> +	  gdb::observers::parameter_changed.notify
>  	    (name.c_str (), c->var->get<std::string> ().c_str ());
>  	  break;
>  	case var_enum:
> -	  interps_notify_param_changed
> +	  gdb::observers::parameter_changed.notify
>  	    (name.c_str (), c->var->get<const char *> ());
>  	  break;
>  	case var_color:
>  	  {
>  	    const ui_file_style::color &color
>  	      = c->var->get<ui_file_style::color> ();
> -	    interps_notify_param_changed
> +	    gdb::observers::parameter_changed.notify
>  	      (name.c_str (), color.to_string ().c_str ());
>  	  }
>  	  break;
> @@ -505,7 +505,7 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
>  	  {
>  	    const char *opt = c->var->get<bool> () ? "on" : "off";
>  
> -	    interps_notify_param_changed (name.c_str (), opt);
> +	    gdb::observers::parameter_changed.notify (name.c_str (), opt);
>  	  }
>  	  break;
>  	case var_auto_boolean:
> @@ -513,7 +513,7 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
>  	    const char *s
>  	      = auto_boolean_enums[c->var->get<enum auto_boolean> ()];
>  
> -	    interps_notify_param_changed (name.c_str (), s);
> +	    gdb::observers::parameter_changed.notify (name.c_str (), s);
>  	  }
>  	  break;
>  	case var_uinteger:
> @@ -521,7 +521,7 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
>  	    char s[64];
>  
>  	    xsnprintf (s, sizeof s, "%u", c->var->get<unsigned int> ());
> -	    interps_notify_param_changed (name.c_str (), s);
> +	    gdb::observers::parameter_changed.notify (name.c_str (), s);
>  	  }
>  	  break;
>  	case var_integer:
> @@ -530,7 +530,7 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
>  	    char s[64];
>  
>  	    xsnprintf (s, sizeof s, "%d", c->var->get<int> ());
> -	    interps_notify_param_changed (name.c_str (), s);
> +	    gdb::observers::parameter_changed.notify (name.c_str (), s);
>  	  }
>  	  break;
>  	}
> diff --git a/gdb/interps.c b/gdb/interps.c
> index b4f564a046a05ad39e20aa3270c60ff2d92f79b4..6c5427842651b795f38f5bed6856739acbbbbba0 100644
> --- a/gdb/interps.c
> +++ b/gdb/interps.c
> @@ -39,6 +39,7 @@
>  #include "main.h"
>  #include "gdbsupport/buildargv.h"
>  #include "gdbsupport/scope-exit.h"
> +#include "observable.h"
>  
>  /* The magic initialization routine for this module.  */
>  
> @@ -589,4 +590,7 @@ A command can have arguments, separated by spaces.\n\
>  These spaces must be escaped using \\ or the command\n\
>  and its arguments must be enclosed in double quotes."), &cmdlist);
>    set_cmd_completer (c, interpreter_completer);
> +
> +  gdb::observers::parameter_changed.attach (interps_notify_param_changed,
> +					    "interpreters");
>  }
> diff --git a/gdb/observable.c b/gdb/observable.c
> index 8439f11c15deebb26aea4c9d9f7cc1d6a929f73f..fd5ddbbfe5424415195f99df3b97c0d34592adb1 100644
> --- a/gdb/observable.c
> +++ b/gdb/observable.c
> @@ -77,6 +77,7 @@ DEFINE_OBSERVABLE (new_program_space);
>  DEFINE_OBSERVABLE (free_program_space);
>  DEFINE_OBSERVABLE (tui_enabled);
>  DEFINE_OBSERVABLE (core_file_changed);
> +DEFINE_OBSERVABLE (parameter_changed);
>  
>  } /* namespace observers */
>  } /* namespace gdb */
> diff --git a/gdb/observable.h b/gdb/observable.h
> index 5f064cf1fc8c2d58c37eb2253fe252f5279d4995..1775c18156b643e8e182f73bd5517aa37b41624e 100644
> --- a/gdb/observable.h
> +++ b/gdb/observable.h
> @@ -266,6 +266,13 @@ extern observable<bool /* enabled */> tui_enabled;
>     shared libraries have been loaded, registers and threads read in, etc.  */
>  extern observable<inferior */* inf */> core_file_changed;
>  
> +/* The parameter ("set" command) with the given name has changed.  The
> +   new value is given in string form.  NAME is the full name of the
> +   parameter, with prefix elements as well.  This is only called for
> +   ordinary parameters -- not maintenance/obscure parameters.  */
> +extern observable<const char */* name */, const char */* value */>
> +     parameter_changed;
> +
>  } /* namespace observers */
>  
>  } /* namespace gdb */
>
> -- 
> 2.49.0
  

>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

Andrew> Tom Tromey <tom@tromey.com> writes:
>> This adds a parameter_changed observer and has it fire in
>> do_set_command.  The interpreter code is updated to use this
>> mechanism.

Andrew> Is there a reason why the TUI interpreter couldn't just be wired up to
Andrew> listen for the ::param_changed interpreter call?  In general the TUI
Andrew> seems to be using observers over the interps_notify_* function API.  I
Andrew> wonder why that is?

In my opinion, observers are the better solution, all other things being
equal.

With an observer, a given TUI window can localize the decision of
whether or not an event must be listened to.  That is, it can register
an observer when the window is created, keep all the logic in private
methods, and unregister the observer when the window is destroyed.

Using the interp method on the other hand means that at least some logic
has to be in the TUI interpreter code.  And in the past the TUI had a
ton of spaghetti code due (IMO) to approaches like this.

Andrew> I'm OK with adding an observer here though, as we might want to expose
Andrew> this in the Python API (later on, not required in this patch), as user
Andrew> extensions might want to respond to setting changes too.

Yeah.

Andrew> However, I think we should follow the model of, for example,
Andrew> notify_breakpoint_deleted, where we have a single function which calls
Andrew> the interps_notify_param_changed function, followed by triggering the
Andrew> observers.

Andrew> I guess in this case it likely doesn't matter, but the existing code
Andrew> (for e.g. breakpoints) is the way it is because we want interpreters to
Andrew> see the events before other general observers (which is why I'm curious
Andrew> that TUI uses observers, not the interps method API).

An ordering like this should probably come with some kind of comment
explaining the need for it.  I don't see one in
notify_breakpoint_modified, which I assume is an oversight; but for the
parameter-changed event, I don't even think this is needed.

Tom