Re: [PATCH 1/6] new observer command_option_changed.

[Tom Tromey <tromey@redhat.com>]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

Yao> This patch is to add an extra field 'notify_observer_p' to identify
Yao> the command will notify observer 'command_option_changed', and teach
Yao> GDB to check the change of command option, and notify observer.

I read through the series to find out why this was needed.
My understanding is that the idea is that options will be segregated
into two kinds: those which report via the observer, and others; and
that the decision is made in the gdb sources.

If that is accurate, I wonder if you could explain the rationale for it.
Another possible design would be to report all option changes; or to let
the MI client choose.

The reason I ask is that making the decision statically seems tricky.
When adding an option, what criteria do we use to decide?  How do we
know what MI users want?  Or what if we want to add a Python hook
here -- maybe we would switch many more options over; but does switching
an option from non-reporting to reporting cause issues for existing MI
clients?  (I assume there is at least a bandwidth concern...)

Yao> +const char *auto_boolean_enums[] = { "on", "off", "auto", NULL };

It seems like now this should be 'const char * const'.
I realize you're just moving it, so normally it would be exempt from
this kind of tweakery, but in this case it is also now exported...

Yao> +		if (c->notify_observer_p)
Yao> +		  {
Yao> +		    char *s = (char *) auto_boolean_enums[val];

I don't think the cast is needed.

Yao> +@deftypefun void command_option_changed (const char *@var{command}, const char *@var{option})
Yao> +The option of some @code{set} commands in console are changed.  This method is called after
Yao> +a command @code{set command option}.
Yao> +@end deftypefun

The lines in the body are all too long.

I think it would be good to describe the meaning of the arguments.


What happens in the case where an option has a validation function that
fails?  IIRC gdb has an internal design flaw here.

Tom