From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 13109 invoked by alias); 18 Mar 2002 00:55:23 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 12859 invoked from network); 18 Mar 2002 00:55:16 -0000 Received: from unknown (HELO localhost.redhat.com) (24.112.135.44) by sources.redhat.com with SMTP; 18 Mar 2002 00:55:16 -0000 Received: from cygnus.com (localhost [127.0.0.1]) by localhost.redhat.com (Postfix) with ESMTP id 6A1683EC7; Sun, 17 Mar 2002 19:55:12 -0500 (EST) Message-ID: <3C953AF0.7040504@cygnus.com> Date: Sun, 17 Mar 2002 16:55:00 -0000 From: Andrew Cagney User-Agent: Mozilla/5.0 (X11; U; NetBSD macppc; en-US; rv:0.9.8) Gecko/20020210 X-Accept-Language: en-us MIME-Version: 1.0 To: gdb-patches@sources.redhat.com, Fernando Nasser Subject: [rfc/cli:rfa] Don't copy func() into show from set .. Content-Type: multipart/mixed; boundary="------------030607060300070401080300" X-SW-Source: 2002-03/txt/msg00291.txt.bz2 This is a multi-part message in MIME format. --------------030607060300070401080300 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit Content-length: 1742 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). --------------030607060300070401080300 Content-Type: text/plain; name="diffs" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="diffs" Content-length: 4602 2002-03-17 Andrew Cagney * 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. */ --------------030607060300070401080300--