PR cli/21688: Fix multi-line/inline command differentiation
Commit Message
On Thursday, June 29 2017, Simon Marchi wrote:
> On 2017-06-29 21:48, Sergio Durigan Junior wrote:
>> On Thursday, June 29 2017, Simon Marchi wrote:
>>> Another (maybe simpler) way would be to check
>>>
>>> else if (command_name_equals (cmd, "python") && *cmd_name == '\0')
>>>
>>> It's not clear when expressed like this though because cmd_name is not
>>> well named at this point (it points just after the command name).
>>
>> Hm, right. Would you prefer this way instead? I don't have a strong
>> opinion on this.
>
> My opinion is the solution with the least code is probably best, if
> they are equivalent otherwise, but I don't really mind. It's just a
> suggestion.
Right. I did some more tests here, and unfortunately your solution
doesn't work for all cases. For example, if the user puts trailing
whitespace on the command name (like "python "), *cmd_name will point to
a whitespace after the call to lookup_cmd_1.
So here's second version of the patch, with the fixes you requested
except the one above. WDYT?
Comments
On 2017-06-30 00:21, Sergio Durigan Junior wrote:
> On Thursday, June 29 2017, Simon Marchi wrote:
>
>> On 2017-06-29 21:48, Sergio Durigan Junior wrote:
>>> On Thursday, June 29 2017, Simon Marchi wrote:
>>>> Another (maybe simpler) way would be to check
>>>>
>>>> else if (command_name_equals (cmd, "python") && *cmd_name == '\0')
>>>>
>>>> It's not clear when expressed like this though because cmd_name is
>>>> not
>>>> well named at this point (it points just after the command name).
>>>
>>> Hm, right. Would you prefer this way instead? I don't have a strong
>>> opinion on this.
>>
>> My opinion is the solution with the least code is probably best, if
>> they are equivalent otherwise, but I don't really mind. It's just a
>> suggestion.
>
> Right. I did some more tests here, and unfortunately your solution
> doesn't work for all cases. For example, if the user puts trailing
> whitespace on the command name (like "python "), *cmd_name will point
> to
> a whitespace after the call to lookup_cmd_1.
Ah, I got confused because there's some code that strips trailing
whitespaces, but it only set p_end, it doesn't modify the string.
> So here's second version of the patch, with the fixes you requested
> except the one above. WDYT?
That LGTM.
Simon
On 06/29/2017 11:21 PM, Sergio Durigan Junior wrote:
> +/* Return true if NAME is the only command between COMMAND_START and
> + COMMAND_END. This is useful when we want to know whether the
> + command is inline (i.e., has arguments like 'python command1') or
> + is the start of a multi-line command block. */
> +
> +static bool
> +command_name_equals_not_inline (const char *command_start,
> + const char *command_end,
> + const char *name)
> +{
> + return (command_end - command_start == strlen (name)
> + && startswith (command_start, name));
> +}
...
> - else if (command_name_equals (cmd, "python"))
> + else if (command_name_equals_not_inline (p_start, p_end, "python"))
> {
Does this handle command aliases? It doesn't look like it.
> + set define_cmd_inline {
> + { "if 1" " >$" "inline if 1" }
> + { "python print ('hello')" " >$" "inline python command" }
For example, what if you write instead:
{ "py print ('hello')" " >$" "inline python command" }
and/or you do:
(gdb) alias foo=python
and then:
{ "foo print ('hello')" " >$" "inline python command" }
Thanks,
Pedro Alves
On Friday, June 30 2017, Simon Marchi wrote:
> On 2017-06-30 00:21, Sergio Durigan Junior wrote:
>> On Thursday, June 29 2017, Simon Marchi wrote:
>>
>>> On 2017-06-29 21:48, Sergio Durigan Junior wrote:
>>>> On Thursday, June 29 2017, Simon Marchi wrote:
>>>>> Another (maybe simpler) way would be to check
>>>>>
>>>>> else if (command_name_equals (cmd, "python") && *cmd_name == '\0')
>>>>>
>>>>> It's not clear when expressed like this though because cmd_name
>>>>> is not
>>>>> well named at this point (it points just after the command name).
>>>>
>>>> Hm, right. Would you prefer this way instead? I don't have a strong
>>>> opinion on this.
>>>
>>> My opinion is the solution with the least code is probably best, if
>>> they are equivalent otherwise, but I don't really mind. It's just a
>>> suggestion.
>>
>> Right. I did some more tests here, and unfortunately your solution
>> doesn't work for all cases. For example, if the user puts trailing
>> whitespace on the command name (like "python "), *cmd_name will
>> point to
>> a whitespace after the call to lookup_cmd_1.
>
> Ah, I got confused because there's some code that strips trailing
> whitespaces, but it only set p_end, it doesn't modify the string.
Yeah. Another option would be to advance cmd_name until there is no
more whitespace-like char. Anyway...
>> So here's second version of the patch, with the fixes you requested
>> except the one above. WDYT?
>
> That LGTM.
Thanks, pushed.
51ed89aa0dce3db46561235efdc4bbc0661bcf37
On Friday, June 30 2017, Pedro Alves wrote:
> On 06/29/2017 11:21 PM, Sergio Durigan Junior wrote:
>> +/* Return true if NAME is the only command between COMMAND_START and
>> + COMMAND_END. This is useful when we want to know whether the
>> + command is inline (i.e., has arguments like 'python command1') or
>> + is the start of a multi-line command block. */
>> +
>> +static bool
>> +command_name_equals_not_inline (const char *command_start,
>> + const char *command_end,
>> + const char *name)
>> +{
>> + return (command_end - command_start == strlen (name)
>> + && startswith (command_start, name));
>> +}
>
> ...
>
>> - else if (command_name_equals (cmd, "python"))
>> + else if (command_name_equals_not_inline (p_start, p_end, "python"))
>> {
>
> Does this handle command aliases? It doesn't look like it.
Hm, no, it doesn't. I guess that the best approach would be to make
sure that lookup_cmd_1 advances the **text pointer past all the
whitespace chars after it matches a command, and then we could use
Simon's idea and check for *cmd_name != '\0'.
I'll prepare a patch here and do some testings.
On 06/30/2017 12:24 PM, Sergio Durigan Junior wrote:
> Hm, no, it doesn't. I guess that the best approach would be to make
> sure that lookup_cmd_1 advances the **text pointer past all the
> whitespace chars after it matches a command, and then we could use
> Simon's idea and check for *cmd_name != '\0'.
I don't see the point of touching lookup_cmd_1, and then
handling fallout of that.
Simply do this after the lookup_cmd_1 call:
lookup_cmd = skip_spaces_const (cmd_name);
bool inline_cmd = *cmd_name != '\0';
and then you can do:
else if (command_name_equals (cmd, "python") && !inline_cmd)
?
>
> I'll prepare a patch here and do some testings.
>
On Friday, June 30 2017, Pedro Alves wrote:
> On 06/30/2017 12:24 PM, Sergio Durigan Junior wrote:
>
>> Hm, no, it doesn't. I guess that the best approach would be to make
>> sure that lookup_cmd_1 advances the **text pointer past all the
>> whitespace chars after it matches a command, and then we could use
>> Simon's idea and check for *cmd_name != '\0'.
>
> I don't see the point of touching lookup_cmd_1, and then
> handling fallout of that.
>
> Simply do this after the lookup_cmd_1 call:
>
> lookup_cmd = skip_spaces_const (cmd_name);
> bool inline_cmd = *cmd_name != '\0';
>
> and then you can do:
>
> else if (command_name_equals (cmd, "python") && !inline_cmd)
>
> ?
Indeed, that is much easier. I'll send a patch soon.
@@ -1,3 +1,10 @@
+2017-06-29 Sergio Durigan Junior <sergiodj@redhat.com>
+
+ PR cli/21688
+ * cli/cli-script.c (command_name_equals_not_inline): New function.
+ (process_next_line): Adjust 'if' clauses for "python", "compile"
+ and "guile" to use command_name_equals_not_inline.
+
2017-06-29 Pedro Alves <palves@redhat.com>
* completer.c (expression_completer): Call
@@ -900,6 +900,20 @@ command_name_equals (struct cmd_list_element *cmd, const char *name)
&& strcmp (cmd->name, name) == 0);
}
+/* Return true if NAME is the only command between COMMAND_START and
+ COMMAND_END. This is useful when we want to know whether the
+ command is inline (i.e., has arguments like 'python command1') or
+ is the start of a multi-line command block. */
+
+static bool
+command_name_equals_not_inline (const char *command_start,
+ const char *command_end,
+ const char *name)
+{
+ return (command_end - command_start == strlen (name)
+ && startswith (command_start, name));
+}
+
/* Given an input line P, skip the command and return a pointer to the
first argument. */
@@ -997,21 +1011,20 @@ process_next_line (char *p, struct command_line **command, int parse_commands,
{
*command = build_command_line (commands_control, line_first_arg (p));
}
- else if (command_name_equals (cmd, "python"))
+ else if (command_name_equals_not_inline (p_start, p_end, "python"))
{
/* Note that we ignore the inline "python command" form
here. */
*command = build_command_line (python_control, "");
}
- else if (command_name_equals (cmd, "compile"))
+ else if (command_name_equals_not_inline (p_start, p_end, "compile"))
{
/* Note that we ignore the inline "compile command" form
here. */
*command = build_command_line (compile_control, "");
(*command)->control_u.compile.scope = COMPILE_I_INVALID_SCOPE;
}
-
- else if (command_name_equals (cmd, "guile"))
+ else if (command_name_equals_not_inline (p_start, p_end, "guile"))
{
/* Note that we ignore the inline "guile command" form here. */
*command = build_command_line (guile_control, "");
@@ -1,3 +1,9 @@
+2017-06-29 Sergio Durigan Junior <sergiodj@redhat.com>
+
+ PR cli/21688
+ * gdb.python/py-cmd.exp (test_python_inline_or_multiline): New
+ procedure. Call it.
+
2017-06-29 Pedro Alves <palves@redhat.com>
* gdb.base/printcmds.exp: Add tests.
@@ -181,6 +181,38 @@ gdb_test "complete expr_test bar\." \
"expr_test bar\.bc.*expr_test bar\.ij.*" \
"test completion through complete command"
+# Test that the "python" command is correctly recognized as
+# inline/multi-line when entering a sequence of commands.
+#
+# This proc tests PR cli/21688. The PR is not language-specific, but
+# the easiest way is just to test with Python.
+proc test_python_inline_or_multiline { } {
+ set define_cmd_not_inline {
+ { "if 1" " >$" "multi-line if 1" }
+ { "python" " >$" "multi-line python command" }
+ { "print ('hello')" " >$" "multi-line print" }
+ { "end" " >$" "multi-line first end" }
+ { "end" "hello\r\n" "multi-line last end" } }
+
+ set define_cmd_inline {
+ { "if 1" " >$" "inline if 1" }
+ { "python print ('hello')" " >$" "inline python command" }
+ { "end" "hello\r\n" "inline end" } }
+
+ foreach t [list $define_cmd_not_inline $define_cmd_inline] {
+ foreach l $t {
+ lassign $l command regex testmsg
+ gdb_test_multiple "$command" "$testmsg" {
+ -re "$regex" {
+ pass "$testmsg"
+ }
+ }
+ }
+ }
+}
+
+test_python_inline_or_multiline
+
if { [readline_is_used] } {
set test "complete 'expr_test bar.i'"
send_gdb "expr_test bar\.i\t\t"