This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 2/3] Error out immediatly when using if command without args in command list
- From: Pedro Alves <palves at redhat dot com>
- To: Simon Marchi <simon dot marchi at ericsson dot com>, gdb-patches at sourceware dot org
- Date: Mon, 4 Sep 2017 13:35:47 +0100
- Subject: Re: [PATCH 2/3] Error out immediatly when using if command without args in command list
- Authentication-results: sourceware.org; auth=none
- Authentication-results: ext-mx06.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com
- Authentication-results: ext-mx06.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=palves at redhat dot com
- Dmarc-filter: OpenDMARC Filter v1.3.2 mx1.redhat.com C14363DBC5
- References: <1504388179-579-1-git-send-email-simon.marchi@ericsson.com> <1504388179-579-3-git-send-email-simon.marchi@ericsson.com>
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.
> --- 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'
Thanks,
Pedro Alves