From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 71288 invoked by alias); 4 Sep 2017 17:15:35 -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 71154 invoked by uid 89); 4 Sep 2017 17:15:28 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-11.3 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_2,GIT_PATCH_3,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: smtp.polymtl.ca Received: from smtp.polymtl.ca (HELO smtp.polymtl.ca) (132.207.4.11) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 04 Sep 2017 17:15:21 +0000 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id v84HFBWe022449 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Mon, 4 Sep 2017 13:15:16 -0400 Received: by simark.ca (Postfix, from userid 112) id ACBF81EA26; Mon, 4 Sep 2017 13:15:11 -0400 (EDT) Received: from simark.ca (localhost [127.0.0.1]) by simark.ca (Postfix) with ESMTP id 9BE341E974; Mon, 4 Sep 2017 13:15:10 -0400 (EDT) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Mon, 04 Sep 2017 17:15:00 -0000 From: Simon Marchi To: Pedro Alves Cc: Simon Marchi , gdb-patches@sourceware.org Subject: Re: [PATCH 2/3] Error out immediatly when using if command without args in command list In-Reply-To: References: <1504388179-579-1-git-send-email-simon.marchi@ericsson.com> <1504388179-579-3-git-send-email-simon.marchi@ericsson.com> Message-ID: <56cb55e5c6eace9d680d432d68cfcda9@polymtl.ca> X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.3.0 X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Mon, 4 Sep 2017 17:15:11 +0000 X-IsSubscribed: yes X-SW-Source: 2017-09/txt/msg00058.txt.bz2 On 2017-09-04 14:35, Pedro Alves wrote: > On 09/02/2017 10:36 PM, Simon Marchi wrote: >> When using "if" (or while) without args directly on gdb's command >> line, >> you get this: >> >> (gdb) if >> if/while commands require arguments >> >> When doing the same when entering a command list, you only get an >> error >> when the command is executed, when parse_exp_in_context_1 fails to >> evaluate the expression. >> >> (gdb) define foo >> Type commands for definition of "foo". >> End with a line saying just "end". >> >if >> >end >> >end >> (gdb) foo >> Argument required (expression to compute). >> >> I think it would make more sense to error out when inputting the >> command >> list directly: >> >> (gdb) define foo >> Type commands for definition of "foo". >> End with a line saying just "end". >> >if >> if/while commands require arguments. >> >> The only required change is to check whether args is an empty string >> in >> build_command_line. >> > > LGTM. Tiny nit further below. > > BTW, as a potential improvement, we could consider also not > canceling the whole command definition, but instead go back to > expecting another line. It's a bit annoying to have to type > everything from scratch. I've run into that occasionally with > tracepoints, like: > > (gdb) trace foo > (gdb) actions > Enter actions for tracepoint 1, one per line. > End with a line saying just "end". > >collect ... > >collect ... > > #... several lines later: > >endd # whoops, a typo. > `endd' is not a tracepoint action, or is ambiguous. > (gdb) # bah, have to start over. > > Instead of: > > (gdb) trace foo > (gdb) actions > Enter actions for tracepoint 1, one per line. > End with a line saying just "end". > >collect ... > >collect ... > > #... several lines later: > >endd > `endd' is not a tracepoint action, or is ambiguous. > >end > (gdb) > > The same safety net applied to if/while typos might be useful. > Just an idea. I thought about the same thing. We just need to make it clear that the erroneous command didn't make it in the command list. >> --- a/gdb/cli/cli-script.c >> +++ b/gdb/cli/cli-script.c >> @@ -147,7 +147,8 @@ build_command_line (enum command_control_type >> type, const char *args) >> { >> struct command_line *cmd; >> >> - if (args == NULL && (type == if_control || type == while_control)) >> + if ((args == NULL || strlen (args) == 0) >> + && (type == if_control || type == while_control)) >> error (_("if/while commands require arguments.")); >> gdb_assert (args != NULL); > > Nit: might not make a difference with modern compilers, though > the canonical way to check for entry string would be: > > *args == '\0' Ok, I'm pushing now with that changed. Thanks, Simon