From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 56234 invoked by alias); 19 Jan 2017 14:55:36 -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 56214 invoked by uid 89); 19 Jan 2017 14:55:34 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-5.1 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=10277, guittonadacorecom, sk:guitton, guitton@adacore.com 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 ESMTP; Thu, 19 Jan 2017 14:55:24 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 8D59F76E5; Thu, 19 Jan 2017 14:55:24 +0000 (UTC) Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.phx2.redhat.com [10.5.9.4]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v0JEtMkL007636; Thu, 19 Jan 2017 09:55:23 -0500 Subject: Re: [RFA] completion in command definition To: Jerome Guitton , Simon Marchi References: <1484058481-6378-1-git-send-email-guitton@adacore.com> <81874bfd1c99e89162b98d18fa8cb385@polymtl.ca> <20170111160518.GH27546@adacore.com> <677c2a720f77b21fd672c24efc41c878@polymtl.ca> <20170112100506.GK27546@adacore.com> Cc: gdb-patches@sourceware.org From: Pedro Alves Message-ID: <41f19570-04ab-cd04-eeb3-77170d258c43@redhat.com> Date: Thu, 19 Jan 2017 14:55:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <20170112100506.GK27546@adacore.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-01/txt/msg00383.txt.bz2 Hi Jerome, On 01/12/2017 10:05 AM, Jerome Guitton wrote: > commit 05bd4353619a49e8506bc6f8cd428b6a0961b28f > Author: Jerome Guitton > Date: Tue Jan 10 15:15:53 2017 +0100 > > [RFA] completion in command definition > > When defining a new macro, "command" is not recognized as an aliased for > "commands": > > (gdb) define breakmain > Type commands for definition of "breakmain". > End with a line saying just "end". > >break main > >command > >echo "IN MAIN\n" > >end > (gdb) > > There is a special case for while-stepping, where 'ws' and 'stepping' are > recognized explicitely. We could add "command" to the list as well. I'd > rather use cli-decode though. Patch in attachment, with a test, tested > in x86-linux. OK to apply? OK (but please remember to edit out the question and "attachment" from the commit log). > > gdb/ChangeLog: > * cli/cli-decode.c (find_command_name_length): Make it global. s/global/extern/ > * cli/cli-decode.h (find_command_name_length): Declare. > * cli/cli-script.c (command_name_equals, line_first_arg): > New functions. > (process_next_line): Use cli-decode to parse command names Missing period. > > gdb/testsuite/ChangeLog: > > * gdb.base/define.exp: Add test for completion in command definition. > > diff --git a/gdb/cli/cli-decode.c b/gdb/cli/cli-decode.c > index dbd874e..b71f6f6 100644 > --- a/gdb/cli/cli-decode.c > +++ b/gdb/cli/cli-decode.c > @@ -1255,7 +1255,7 @@ find_cmd (const char *command, int len, struct cmd_list_element *clist, > return found; > } > > -static int > +int > find_command_name_length (const char *text) > { > const char *p = text; > diff --git a/gdb/cli/cli-decode.h b/gdb/cli/cli-decode.h > index e5ab839..66159fd 100644 > --- a/gdb/cli/cli-decode.h > +++ b/gdb/cli/cli-decode.h > @@ -253,4 +253,6 @@ extern const char * const auto_boolean_enums[]; > > extern int cli_user_command_p (struct cmd_list_element *); > > +extern int find_command_name_length (const char *); > + Now that this is a public function, it'd be nice to document its interface. > #endif /* !defined (CLI_DECODE_H) */ > diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c > index 6f44d52..f19df9e 100644 > --- a/gdb/cli/cli-script.c > +++ b/gdb/cli/cli-script.c > @@ -143,7 +143,7 @@ multi_line_command_p (enum command_control_type type) > control commands (if/while). */ > > static struct command_line * > -build_command_line (enum command_control_type type, char *args) > +build_command_line (enum command_control_type type, const char *args) Mention this in the ChangeLog. > { > struct command_line *cmd; > > @@ -904,6 +904,30 @@ read_next_line (void) > return command_line_input (prompt_ptr, from_tty, "commands"); > } > > +/* Return non-zero if CMD's name is NAME. */ > + > +static bool > +command_name_equals (struct cmd_list_element *cmd, const char *name) > +{ > + return cmd != NULL > + && cmd != CMD_LIST_AMBIGUOUS > + && strcmp (cmd->name, name) == 0; Multi-line conditionals should be wrapped in ()s, and then the && naturally aligns under "cmd": return (cmd != NULL && cmd != CMD_LIST_AMBIGUOUS && strcmp (cmd->name, name) == 0); (and suitably tabified). > +} > + > +/* Given an input line P, skip the command and return a pointer to the > + first argument. */ > + > +static const char * > +line_first_arg (const char *p) > +{ > + const char *first_arg = p + find_command_name_length (p); > + > + while (first_arg != '\0' && isspace (*first_arg)) > + first_arg++; return skip_spaces_const (first_arg); ? > + > + return first_arg; > +} > + > /* Process one input line. If the command is an "end", return such an > indication to the caller. If PARSE_COMMANDS is true, strip leading > whitespace (trailing whitespace is always stripped) in the line, > @@ -919,6 +943,8 @@ process_next_line (char *p, struct command_line **command, int parse_commands, > char *p_end; > char *p_start; > int not_handled = 0; > + const char *cmd_name = p; > + struct cmd_list_element *cmd; Move these to the scope that needs them? > > /* Not sure what to do here. */ > if (p == NULL) > @@ -938,9 +964,11 @@ process_next_line (char *p, struct command_line **command, int parse_commands, > We also permit whitespace before end and after. */ > if (p_end - p_start == 3 && startswith (p_start, "end")) > return end_command; > - > + > if (parse_commands) > { > + cmd = lookup_cmd_1 (&cmd_name, cmdlist, NULL, 1); It'd be good to add a comment about why we do this instead of just doing a strcmp. Mention something about aliases and unique command abbreviations. Please tweak the subject line / commit log too; I initially thought this was going to be about tab completion. :-) > + > /* If commands are parsed, we skip initial spaces. Otherwise, > which is the case for Python commands and documentation > (see the 'document' command), spaces are preserved. */ > @@ -958,9 +986,7 @@ process_next_line (char *p, struct command_line **command, int parse_commands, > > /* Check for while, if, break, continue, etc and build a new > command line structure for them. */ > - if ((p_end - p >= 14 && startswith (p, "while-stepping")) > - || (p_end - p >= 8 && startswith (p, "stepping")) > - || (p_end - p >= 2 && startswith (p, "ws"))) > + if (command_name_equals (cmd, "while-stepping")) > { > /* Because validate_actionline and encode_action lookup > command's line as command, we need the line to > @@ -975,40 +1001,25 @@ process_next_line (char *p, struct command_line **command, int parse_commands, > not. */ > *command = build_command_line (while_stepping_control, p); > } > - else if (p_end - p > 5 && startswith (p, "while")) > + else if (command_name_equals (cmd, "while")) > { > - char *first_arg; > - > - first_arg = p + 5; > - while (first_arg < p_end && isspace (*first_arg)) > - first_arg++; > - *command = build_command_line (while_control, first_arg); > + *command = build_command_line (while_control, line_first_arg (p)); > } > - else if (p_end - p > 2 && startswith (p, "if")) > + else if (command_name_equals (cmd, "if")) > { > - char *first_arg; > - > - first_arg = p + 2; > - while (first_arg < p_end && isspace (*first_arg)) > - first_arg++; > - *command = build_command_line (if_control, first_arg); > + *command = build_command_line (if_control, line_first_arg (p)); > } > - else if (p_end - p >= 8 && startswith (p, "commands")) > + else if (command_name_equals (cmd, "commands")) > { > - char *first_arg; > - > - first_arg = p + 8; > - while (first_arg < p_end && isspace (*first_arg)) > - first_arg++; > - *command = build_command_line (commands_control, first_arg); > + *command = build_command_line (commands_control, line_first_arg (p)); > } > - else if (p_end - p == 6 && startswith (p, "python")) > + else if (command_name_equals (cmd, "python")) > { > /* Note that we ignore the inline "python command" form > here. */ > *command = build_command_line (python_control, ""); > } > - else if (p_end - p == 6 && startswith (p, "compile")) > + else if (command_name_equals (cmd, "compile")) > { > /* Note that we ignore the inline "compile command" form > here. */ > @@ -1016,7 +1027,7 @@ process_next_line (char *p, struct command_line **command, int parse_commands, > (*command)->control_u.compile.scope = COMPILE_I_INVALID_SCOPE; > } > > - else if (p_end - p == 5 && startswith (p, "guile")) > + else if (command_name_equals (cmd, "guile")) > { > /* Note that we ignore the inline "guile command" form here. */ > *command = build_command_line (guile_control, ""); > diff --git a/gdb/testsuite/gdb.base/define.exp b/gdb/testsuite/gdb.base/define.exp > index 59203ec..b57b068 100644 > --- a/gdb/testsuite/gdb.base/define.exp > +++ b/gdb/testsuite/gdb.base/define.exp > @@ -147,6 +147,30 @@ gdb_test_multiple "define ifnospace" "define user command: ifnospace" \ > > gdb_test "ifnospace" ".*hi there.*" "test ifnospace is parsed correctly" > > +# Verify that the command parser properly handle completion. Here too. > +set test "define use command: breakmain" typo: "user" ? > +gdb_test_multiple "define breakmain" "$test" \ > +{ > + -re "Type commands for definition of \"breakmain\".\r\nEnd with a line saying just \"end\".\r\n>$" \ > + { > + pass "$test" > + > + set test "send body of breakmain" > + gdb_test_multiple "break main\ncommand\necho\nend\nend" "$test" \ > + { > + -re "$gdb_prompt $"\ > + {pass "$test"} Does this fail with an unfixed GDB? I ask since this is just matching the prompt, which a "bad" GDB also outputs? > + } > + } > +} > + > +gdb_test "breakmain" ".*Breakpoint 2.*" "test command completion in define" Odd message for this test. I suggest wrapping these three tests in with_test_prefix "command abbreviations in define" { set test "define user command: breakmain" gdb_test_multiple "define breakmain" "$test" ..... gdb_test "breakmain" ".*Breakpoint 2.*" "run user command" gdb_test "info break 2" \ .... \ "info break shows echo command" } > + > +gdb_test "info break 2" \ > + "Num Type\[ \]+Disp Enb Address\[ \]+What.* > +\[0-9\]+\[\t \]+breakpoint keep y.* in main at .* > +\[\t \]+echo.*" > + It's better to avoid hardcoding breakpoint numbers, since it'll break if someone adds some test before this one that creates some other breakpoint. You can use "$bpnum" instead to avoid that. > # Verify that the command parser doesn't require a space after an 'while' > # command in a user defined function. > # > Thanks, Pedro Alves