From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 91145 invoked by alias); 10 Jan 2017 16:27:59 -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 91135 invoked by uid 89); 10 Jan 2017 16:27:59 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.2 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_SOFTFAIL autolearn=no version=3.3.2 spammy=jerome, Jerome, H*i:sk:1484058, HX-PHP-Originating-Script:rcube.php X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 10 Jan 2017 16:27:57 +0000 Received: by simark.ca (Postfix, from userid 33) id 77C121E81B; Tue, 10 Jan 2017 11:27:55 -0500 (EST) To: Jerome Guitton Subject: Re: [RFA] completion in command definition X-PHP-Originating-Script: 33:rcube.php MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Tue, 10 Jan 2017 16:27:00 -0000 From: Simon Marchi Cc: gdb-patches@sourceware.org In-Reply-To: <1484058481-6378-1-git-send-email-guitton@adacore.com> References: <1484058481-6378-1-git-send-email-guitton@adacore.com> Message-ID: <81874bfd1c99e89162b98d18fa8cb385@polymtl.ca> X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.2.3 X-IsSubscribed: yes X-SW-Source: 2017-01/txt/msg00170.txt.bz2 Hi Jerome, I can comment on styling issues (some of them nits), but not so much on the actual problem, since I'm not very familiar with it. But in general it cleans up the code nicely I think. > diff --git a/gdb/cli/cli-decode.h b/gdb/cli/cli-decode.h > index e5ab839..83bc34a 100644 > --- a/gdb/cli/cli-decode.h > +++ b/gdb/cli/cli-decode.h > @@ -253,4 +253,5 @@ 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 *); Add new line here? > #endif /* !defined (CLI_DECODE_H) */ > diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c > index 6f44d52..338d726 100644 > --- a/gdb/cli/cli-script.c > +++ b/gdb/cli/cli-script.c > @@ -904,6 +904,28 @@ read_next_line (void) > return command_line_input (prompt_ptr, from_tty, "commands"); > } > > +/* Return non-zero if CMD's name is NAME. */ > + > +static int > +command_name_equals (struct cmd_list_element *cmd, char *name) This can return a bool, and the command can say true instead of non-zero. name can be "const char *". > +{ > + return cmd For null pointer checks, use "cmd != NULL" or "cmd != nullptr". > + && cmd != CMD_LIST_AMBIGUOUS > + && strcmp (cmd->name, name) == 0; > +} > + > +/* Given an input line P, skip the command and return a pointer to the > + first argument. */ > + > +static char * > +line_first_arg (char *p) The argument type and return type can be const. It will require you to constify a few other things which the compiler will tell you. > +{ > + char *first_arg = p + find_command_name_length (p); Add an empty line here... > + while (first_arg != '\0' && isspace (*first_arg)) > + first_arg++; ... and here. > + 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 +941,9 @@ process_next_line (char *p, struct command_line > **command, int parse_commands, > char *p_end; > char *p_start; > int not_handled = 0; > + char *cmd_name = p; This can be constified. > + struct cmd_list_element *last_line = 0; This should be either "= NULL" or "= nullptr", but actually I don't think it needs to be initialized. > + struct cmd_list_element *cmd; > > /* Not sure what to do here. */ > if (p == NULL) > @@ -938,9 +963,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 ((const char **) &cmd_name, cmdlist, > &last_line, 1); If cmd_name is constified, the const char cast can be removed. > diff --git a/gdb/testsuite/gdb.base/define.exp > b/gdb/testsuite/gdb.base/define.exp > index 59203ec..355a7bc 100644 > --- a/gdb/testsuite/gdb.base/define.exp > +++ b/gdb/testsuite/gdb.base/define.exp > @@ -147,6 +147,27 @@ 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. > + > +gdb_test_multiple "define breakmain" "define user command: breakmain" > \ > +{ > + -re "Type commands for definition of \"breakmain\".\r\nEnd with a > line saying just \"end\".\r\n>$" \ > + { > + gdb_test_multiple "break main\ncommand\necho\nend\nend" "send > body of breakmain" \ > + { > + -re "$gdb_prompt $"\ > + {pass "define user command: breakmain"} > + } > + } > +} I think it's preferable to use the same test name in the gdb_test_multiple and in the pass. Usually, we define a variable with the test name and use it for both. Also, if there are two gdb_test_multiple, I guess we would need two "pass"? Like: set test "define user command: breakmain" gdb_test_multiple ... $test { -re ... { pass $test set test "send body of breakmain" gdb_test_multiple ... $test { -re ... { pass $test } } } } I'm not sure about that... Thanks! Simon