Re: [rfc/cli:rfa] Don't copy func() into show from set ..

[Fernando Nasser <fnasser at redhat dot com>]

Andrew, I agree with all that you say before but I have some
additional thoughts.

Should we ever allow for the creation of a set command without
a show?  If not, we should get rid of add_set_cmd() and
add_show_from_set() and have a single add_setshow_cmd().
This would prevent anyone of creating a set without the 
corresponding show and hide the implementation details.

It could return the show pointer as an argument if so requested.

One day we should have an ui independent way to register the
variables and let the UIs create the appropriate commands themselves...

Regards,
Fernando

Andrew Cagney wrote:
> 
> Hello,
> 
> The current add_show_from_set() uses memcpy() to clone the ``set''
> command into a ``show'' command.  The function copies everything
> including ``set''.func(), the command's callback.
> 
> I think doing this is wrong.  The ``show'' command should only pickup
> directly relevant fields from ``set''.  Any others should be set
> separatly and explicitly.
> 
> The first problem I see is with the behavour.  The sequence:
>         c = add_set_cmd (...)
>         set_cmd_sfunc (c, ...)
>         add_show_from_set (c)
> has very different behavour to:
>         c = add_set_cmd (...)
>         add_show_from_set (c)
>         add_cmd_sfunc (c, ...)
> Only in the former case does the ``show'' command get ``set''.func().  I
> think instead a user should be expected to explicitly set
> ``show''.func() vis:
>         c = add_set_cmd (...)
>         s = add_show_from_set (s);
>         set_cmd_sfunc (s, ...)
>         set_cmd_sfunc (c, ...)
> or (i.e. order no longer matters):
>         c = add_set_cmd (...)
>         set_cmd_sfunc (c, ...)
>         s = add_show_from_set (s);
>         set_cmd_sfunc (s, ...)
> 
> The second problem I see is with the unintended consequences.  Because
> ``show'' has silently picked up ``set''.func(), commands like ``info
> set'' call it.  For the most part this is benign.  Since the set
> variable hasn't changed, the ``set''.func() just resets everything back
> to what it was.  Only occasionally has someone noticed this
> ``re-setting'' and found it necessary to ignore it (kod.c, infun.c,
> cris-tdep.c).
> 
> The attatched patch modifies the behavour of add_show_from_set() so that
> it only copies an explicit subset of the fields from the ``set'' command.
> 
> With the patch applied, I've so far found no regressions!
> 
> Thoughts?  Ok?
> 
> Andrew
> 
> PS: This change dates back to before the dawn of time (well at least to
> before Cygnus's CVS repository).
> 


-- 
Fernando Nasser
Red Hat Canada Ltd.                     E-Mail:  fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9