From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29635 invoked by alias); 2 Sep 2014 12:52:09 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 29625 invoked by uid 89); 2 Sep 2014 12:52:09 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.8 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Tue, 02 Sep 2014 12:52:05 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s82Cq06R030821 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Tue, 2 Sep 2014 08:52:01 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s82Cpwhs013086; Tue, 2 Sep 2014 08:51:59 -0400 Message-ID: <5405BD6E.9080000@redhat.com> Date: Tue, 02 Sep 2014 12:52:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.7.0 MIME-Version: 1.0 To: Gabriel Krisman Bertazi CC: gdb-patches@sourceware.org Subject: Re: [PATCH] Fix PR gdb/17035: "show user" doesn't list user-defined commands that have empty bodies. References: <1408515134-31165-1-git-send-email-gabriel@krisman.be> <53F615D8.5000901@redhat.com> <878umgaqjk.fsf@krisman.be> <53FDC751.3000902@redhat.com> <87sikc1p8z.fsf@anubis.Home> In-Reply-To: <87sikc1p8z.fsf@anubis.Home> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2014-09/txt/msg00047.txt.bz2 On 08/31/2014 07:35 PM, Gabriel Krisman Bertazi wrote: > Hello Pedro, > > I appreciate your comments, > >>> >> I don't see that as a problem, because python/scheme are also user >>> >> commands (or, at least, are part of class_user class). Considering that >>> >> "help user-defined" also shows python/scheme commands, I believe it >>> >> would be more practical to just list them on "show user" (should we >>> >> update the doc string, as well). >> > >> > I think we'll need to track down the original discussion, and >> > ask whoever was involved. Otherwise, we'd just be going in >> > circles. :-) >> > >> > E.g., that commit changed the online help and the manual to >> > try to make that clear. >> > >> > (gdb) help show user >> > Show definitions of non-python/scheme user defined commands. >> > Argument is the name of the user defined command. >> > With no argument, show definitions of all user defined commands. >> > > Reviewing the original discussion, (for the record, > ), Doug has > made a pretty good point on not showing python/scheme commands here. > > Since this is a simple bug fix, I'd rather not modify the behavior > here, so I rewrote the patch to not show python/scheme commands. It got > a little simpler, since I don't have to touch a lot of testcases > anymore. > > Now we use the fact that the c->function union is NULL when we have a > python/scheme command (they both use c->func directly). > > I also added two testcases to make sure we are not printing scheme or > python commands, as you suggested. Thanks! > >>> >> Using the verification for cfunc/sfunc above, also avoids >>> >> printing the "user-defined" line. >> > >> > Hmm, but what is that "user-defined" command ? > user-defined is a null command. It does nothing. It is defined as > follows: > > add_cmd ("user-defined", class_user, NULL, _("\ > User-defined commands.\n\ > The commands in this class are those defined by the user.\n\ > Use the \"define\" command to define a command."), &cmdlist); > > I guess it's only purpose is at help user-defined. Yeah. > > Do you think it is acceptable now? Yeah. Close now. Only nits on the details. > > gdb/ > 2014-08-20 Gabriel Krisman Bertazi > > * cli/cli-cmds.c (show_user): Verify if c->function is NULL to > decide whether c is a python/scheme command. > * cli/cli-script.c (show_user_1): Only verify cmdlines after > printing command name. > > gdb/testsuite/ > 2014-08-20 Gabriel Krisman Bertazi > > * gdb.base/commands.exp: Include tests to verify user-defined > commands with empty bodies. > * gdb.python/py-cmd.exp: Don't show user-defined python commands > in `show user command` Prefix that with "Test that we" or some such. Add missing period at the end. > * gdb.python/scm-cmd.exp: Don't show user-defined scheme commands > in `show user command` Likewise. > > > show_user_v2.patch > > > diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c > index b415267..4f624f2 100644 > --- a/gdb/cli/cli-cmds.c > +++ b/gdb/cli/cli-cmds.c > @@ -1245,8 +1245,9 @@ show_user (char *args, int from_tty) > const char *comname = args; > > c = lookup_cmd (&comname, cmdlist, "", 0, 1); > - /* c->user_commands would be NULL if it's a python/scheme command. */ > - if (c->class != class_user || !c->user_commands) > + /* c->function.cfunc would be NULL if it's a python/scheme > + command. */ > + if (c->class != class_user || c->function.cfunc == NULL) > error (_("Not a user command.")); > show_user_1 (c, "", args, gdb_stdout); > } > @@ -1254,7 +1255,8 @@ show_user (char *args, int from_tty) > { > for (c = cmdlist; c; c = c->next) > { > - if (c->class == class_user || c->prefixlist != NULL) > + if ((c->class == class_user && c->function.cfunc != NULL) > - /* c->user_commands would be NULL if it's a python/scheme command. */ > - if (c->class != class_user || !c->user_commands) > + if ((c->class == class_user && c->function.cfunc != NULL) > + || c->prefixlist != NULL) I think it'd be great if we had a small helper function for this predicate. So that we could write something like: if (cli_user_command_p (c)) and: if (cli_user_command_p (c) || c->prefixlist != NULL) Also, best not assume the compiler allows accessing c->function.cfunc when it's c->function.sfunc that is actually active in the union, as in: > + if ((c->class == class_user && c->function.cfunc != NULL) int cli_user_command_p (struct cmd_list_element *cmd) { return (cmd->func == do_cfunc && cmd->function.cfunc != NULL) || cmd->func == do_sfunc && cmd->function.sfunc != NULL); } or just (don't know if it works): int cli_user_command_p (struct cmd_list_element *cmd) { return (cmd->func == do_cfunc || cmd->func == do_sfunc); } > + || c->prefixlist != NULL) > show_user_1 (c, "", c->name, gdb_stdout); > } > } > diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c > index 0f0a97e..37cb82a 100644 > --- a/gdb/cli/cli-script.c > +++ b/gdb/cli/cli-script.c > @@ -1717,10 +1717,10 @@ show_user_1 (struct cmd_list_element *c, const char *prefix, const char *name, > } > > cmdlines = c->user_commands; > - if (!cmdlines) > - return; > fprintf_filtered (stream, "User command \"%s%s\":\n", prefix, name); > > + if (!cmdlines) > + return; > print_command_lines (current_uiout, cmdlines, 1); > fputs_filtered ("\n", stream); > } > diff --git a/gdb/testsuite/gdb.base/commands.exp b/gdb/testsuite/gdb.base/commands.exp > index 7363420..b2d1c76 100644 > --- a/gdb/testsuite/gdb.base/commands.exp > +++ b/gdb/testsuite/gdb.base/commands.exp > @@ -243,6 +243,28 @@ proc user_defined_command_test {} { > gdb_test "show user mycommand" \ > " while \\\$arg0.*set.* if \\\(\\\$arg0.*p/x.* else\[^\n\].*p/x.* end\[^\n\].* end\[^\n\].*" \ > "display user command in user_defined_command_test" > + > + # Create and test an user-defined command with an empty body. s/an user/a user/ > + gdb_test_multiple "define myemptycommand" \ > + "define myemptycommand in user_defined_command_test" { > + -re "End with" { > + pass "define myemptycommand in user_defined_command_test" > + } > + } > + gdb_test "end" \ > + "" \ > + "End definition of user-defined command with empty body." lowercase "end". No period at the end. > + > + gdb_test_no_output "myemptycommand" \ > + "execute user-defined empty command in user_defined_command_test" > + > + gdb_test "show user" \ > + "User command \"myemptycommand.*" \ > + "display empty command in command list in user_defined_command_test" > + > + gdb_test "show user myemptycommand" \ > + "User command \"myemptycommand.*" \ > + "display user-defined emtpy command in user_defined_command_test" Typo: "empty" > proc watchpoint_command_test {} { > diff --git a/gdb/testsuite/gdb.guile/scm-cmd.exp b/gdb/testsuite/gdb.guile/scm-cmd.exp > index a407f63..36ef03d 100644 > --- a/gdb/testsuite/gdb.guile/scm-cmd.exp > +++ b/gdb/testsuite/gdb.guile/scm-cmd.exp > @@ -134,6 +134,10 @@ gdb_test "help user-defined" \ > "User-defined commands.\[\r\n\]+The commands in this class are those defined by the user.\[\r\n\]+Use the \"define\" command to define a command.\[\r\n\]+List of commands:\[\r\n\]+test-help -- Docstring\[\r\n\]+Type \"help\" followed by command name for full documentation.\[\r\n\]+Type \"apropos word\" to search for commands related to \"word\".\[\r\n\]+Command name abbreviations are allowed if unambiguous.\[\r\n\]+" \ > "see user-defined command in `help user-defined`" > > +# Make sure the command does not show up in `show user`. > +gdb_test "show user test-help" "Not a user command\." \ > + "Don't show user-defined scheme command in `show user command`" > + lowercase "don't". > # Test expression completion on fields. > > gdb_test_multiline "expression completion command" \ > diff --git a/gdb/testsuite/gdb.python/py-cmd.exp b/gdb/testsuite/gdb.python/py-cmd.exp > index a87aecb..3ae4053 100644 > --- a/gdb/testsuite/gdb.python/py-cmd.exp > +++ b/gdb/testsuite/gdb.python/py-cmd.exp > @@ -161,6 +161,10 @@ gdb_test "test_help ugh" "test_cmd output, arg = ugh" "call simple user-defined > # Make sure the command shows up in `help user-defined`. > gdb_test "help user-defined" "User-defined commands.\[\r\n\]+The commands in this class are those defined by the user.\[\r\n\]+Use the \"define\" command to define a command.\[\r\n\]+\[\r\n\]+List of commands:\[\r\n\]+\[\r\n\]+test_help -- Docstring\[\r\n\]+\[\r\n\]+Type \"help\" followed by command name for full documentation.\[\r\n\]+Type \"apropos word\" to search for commands related to \"word\".\[\r\n\]+Command name abbreviations are allowed if unambiguous.\[\r\n\]+" "see user-defined command in `help user-defined`" > > +# Make sure the command does not show up in `show user`. > +gdb_test "show user test_help" "Not a user command\." \ > + "Don't show user-defined python command in `show user command`" lowercase "don't". Thanks, Pedro Alves