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

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

[Andrew Cagney]

[-- Attachment #1: Type: text/plain, Size: 1742 bytes --]

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).

[-- Attachment #2: diffs --]
[-- Type: text/plain, Size: 4602 bytes --]

2002-03-17  Andrew Cagney  <ac131313@redhat.com>

	* cli/cli-decode.c: Include "gdb_assert.h".
	(add_setshow_cmd): New function.
	(add_set_cmd): Rewrite.  Use add_setshow_cmd.
	(add_show_from_set): Rewrite. Use add_setshow_cmd.  Don't copy all
	fields, such as func, from the set command.

Index: cli/cli-decode.c
===================================================================
RCS file: /cvs/src/src/gdb/cli/cli-decode.c,v
retrieving revision 1.15
diff -u -r1.15 cli-decode.c
--- cli-decode.c	2002/03/06 06:28:35	1.15
+++ cli-decode.c	2002/03/18 00:16:52
@@ -28,6 +28,8 @@
 #include "cli/cli-cmds.h"
 #include "cli/cli-decode.h"
 
+#include "gdb_assert.h"
+
 /* Prototypes for local functions */
 
 static void undef_cmd_error (char *, char *);
@@ -279,24 +281,26 @@
 {
 }
 
-/* Add element named NAME to command list LIST (the list for set
+/* Add element named NAME to command list LIST (the list for set/show
    or some sublist thereof).
+   TYPE is set_cmd or show_cmd.
    CLASS is as in add_cmd.
    VAR_TYPE is the kind of thing we are setting.
    VAR is address of the variable being controlled by this command.
    DOC is the documentation string.  */
 
-struct cmd_list_element *
-add_set_cmd (char *name,
-	     enum command_class class,
-	     var_types var_type,
-	     void *var,
-	     char *doc,
-	     struct cmd_list_element **list)
+static struct cmd_list_element *
+add_setshow_cmd (char *name,
+		 enum cmd_types type,
+		 enum command_class class,
+		 var_types var_type,
+		 void *var,
+		 char *doc,
+		 struct cmd_list_element **list)
 {
   struct cmd_list_element *c = add_cmd (name, class, NULL, doc, list);
-
-  c->type = set_cmd;
+  gdb_assert (type == set_cmd || type == show_cmd);
+  c->type = type;
   c->var_type = var_type;
   c->var = var;
   /* This needs to be something besides NULL so that this isn't
@@ -305,6 +309,18 @@
   return c;
 }
 
+
+struct cmd_list_element *
+add_set_cmd (char *name,
+	     enum command_class class,
+	     var_types var_type,
+	     void *var,
+	     char *doc,
+	     struct cmd_list_element **list)
+{
+  return add_setshow_cmd (name, set_cmd, class, var_type, var, doc, list);
+}
+
 /* Add element named NAME to command list LIST (the list for set
    or some sublist thereof).
    CLASS is as in add_cmd.
@@ -367,44 +383,30 @@
 }
 
 /* Where SETCMD has already been added, add the corresponding show
-   command to LIST and return a pointer to the added command (not 
+   command to LIST and return a pointer to the added command (not
    necessarily the head of LIST).  */
+/* NOTE: cagney/2002-03-17: The original version of add_show_from_set
+   used memcpy() to clone `set' into `show'.  This ment that in
+   addition to all the needed fields (var, name, et.al.) some
+   unnecessary fields were copied (namely the callback function).  The
+   function explictly copies relevant fields.  For a `set' and `show'
+   command to share the same callback, the caller must set both
+   explicitly.  */
 struct cmd_list_element *
 add_show_from_set (struct cmd_list_element *setcmd,
 		   struct cmd_list_element **list)
 {
-  struct cmd_list_element *showcmd =
-  (struct cmd_list_element *) xmalloc (sizeof (struct cmd_list_element));
-  struct cmd_list_element *p;
-
-  memcpy (showcmd, setcmd, sizeof (struct cmd_list_element));
-  delete_cmd (showcmd->name, list);
-  showcmd->type = show_cmd;
-
-  /* Replace "set " at start of docstring with "show ".  */
-  if (setcmd->doc[0] == 'S' && setcmd->doc[1] == 'e'
-      && setcmd->doc[2] == 't' && setcmd->doc[3] == ' ')
-    showcmd->doc = concat ("Show ", setcmd->doc + 4, NULL);
-  else
-    fprintf_unfiltered (gdb_stderr, "GDB internal error: Bad docstring for set command\n");
-
-  if (*list == NULL || strcmp ((*list)->name, showcmd->name) >= 0)
-    {
-      showcmd->next = *list;
-      *list = showcmd;
-    }
-  else
-    {
-      p = *list;
-      while (p->next && strcmp (p->next->name, showcmd->name) <= 0)
-	{
-	  p = p->next;
-	}
-      showcmd->next = p->next;
-      p->next = showcmd;
-    }
+  char *doc;
+  const static char setstring[] = "Set ";
 
-  return showcmd;
+  /* Create a doc string by replacing "Set " at the start of the
+     `set'' command's doco with "Show ".  */
+  gdb_assert (strncmp (setcmd->doc, setstring, sizeof (setstring) - 1) == 0);
+  doc = concat ("Show ", setcmd->doc + sizeof (setstring) - 1, NULL);
+
+  /* Insert the basic command.  */
+  return add_setshow_cmd (setcmd->name, show_cmd, setcmd->class,
+			  setcmd->var_type, setcmd->var, doc, list);
 }
 
 /* Remove the command named NAME from the command list.  */

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

[Fernando Nasser]

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.
\x18
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

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

[Andrew Cagney]

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

I'll leave it for a week then commit it (if that is ok) .....

> 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.

That's true.

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

Ah, yes!  You're thinking of

	set = add_setshow_cmd (name, class, ..., &show);

where the &show could be NULL.

I can add it, and ARI (just info) the other versions, if you want.

Andrew

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

[Fernando Nasser]

Andrew Cagney wrote:
> 
> > Andrew, I agree with all that you say before but I have some
> > additional thoughts.
> 
> I'll leave it for a week then commit it (if that is ok) .....
> 

Sure.

> > 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.
> 
> That's true.
> 
> > It could return the show pointer as an argument if so requested.
> 
> Ah, yes!  You're thinking of
> 
>         set = add_setshow_cmd (name, class, ..., &show);
> 
> where the &show could be NULL.
> 
> I can add it, and ARI (just info) the other versions, if you want.
> 

If you can, please do it.

Thank you very much for the fix and clean-up.

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

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

[Andrew Cagney]

> Andrew Cagney wrote:
> 
>> 
> 
>> > Andrew, I agree with all that you say before but I have some
>> > additional thoughts.
> 
>> 
>> I'll leave it for a week then commit it (if that is ok) .....
>> 
> 
> 
> Sure.

It is now in.

thanks!
Andrew



2002-03-23  Andrew Cagney  <ac131313@redhat.com>

         * cli/cli-decode.c: Include "gdb_assert.h".
         (add_set_or_show_cmd): New static function.
         (add_set_cmd): Rewrite.  Use add_set_or_show_cmd.
         (add_show_from_set): Rewrite. Use add_set_or_show_cmd.  Don't copy
         all fields, such as func, from the set command.