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

[Andrew Cagney <ac131313 at cygnus dot com>]

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