Re: [PATCH v2 0/4] Introduce the "with" command

[Pedro Alves <palves at redhat dot com>]

On 6/19/19 1:34 AM, Philippe Waroquiers wrote:

>> New in v2:
> I played a little bit with this version, no bug encountered.
> 

Thanks much, again.

> 2 small nits in the error message for unknown 'with settings':
>    (gdb) with xxxx yyyy -- echo coucou
>    Undefined withcommand: "xxxx".  Try "help wit".
>    (gdb) 
> 
> (this message is produced by lookup_cmd, that is not too
> much 'with' aware it seems ...)

Eh, somehow I missed adding a testcase for this scenario.
Thanks for noticing this.

I had code in with_command_1 to throw on unknown setting:

  if (set_cmd == nullptr)
    error (_("Unknown setting %s"), cmd);

but it's not reachable, because I'm passing "0" as
allow_unknown argument to lookup_cmd...  I tried passing
"1" instead, and, of course that error becomes reachable.

However, if the setting is a prefixed setting, like
"with print elements", then lookup_cmd still throws
an error, near the bottom, from the else branch:

      if (c->prefixlist && **line && !c->allow_unknown)
	undef_cmd_error (c->prefixname, *line);

regardless of the allow_unknown parameter.

This made me reconsider, and I'm thinking that indeed,
passing allow_unknown as false is the right thing to do,
so that we get consistent error messages:

 (gdb) with foo
 Undefined set command: "foo".  Try "help set".
 (gdb) with print foo
Undefined set print command: "foo".  Try "help set print".
 (gdb) maint with foo
 Undefined maintenance set command: "foo".  Try "help maintenance set".
 (gdb) maint with test-settings foo
 Undefined maintenance set test-settings command: "foo".  Try "help maintenance set test-settings".

I'm thinking that the errors talking about "set" instead of
"with" can be seen as a feature.  If you consider the prefixed
case, like "with print foo", the error is telling you where
to look at the available settings for that prefix.

Looking at the lookup_cmd code, I realized that I was missing 
a test for ambiguous settings too.  Now added.

The funny missing space and 'h' were because we're supposed
to include a space in the CMDTYPE argument passed to
lookup_cmd, and I was passing "with", with no space.  I added a new
parameter to with_command_1, so that we can pass down
"maintenance with " too.  The completer function already had
the same parameter.

WDYT?

>From 4db38e12c0b393231a7af955a1bd4e9573bfe19e Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Wed, 19 Jun 2019 13:29:08 +0100
Subject: [PATCH] fix

---
 gdb/cli/cli-cmds.c              | 14 +++++++-------
 gdb/cli/cli-cmds.h              |  8 +++++---
 gdb/maint.c                     |  2 +-
 gdb/testsuite/gdb.base/with.exp | 18 ++++++++++++++++++
 4 files changed, 31 insertions(+), 11 deletions(-)

diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index 0f5da72e128..283bc446026 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -214,7 +214,8 @@ show_command (const char *arg, int from_tty)
 /* See cli/cli-cmds.h.  */
 
 void
-with_command_1 (cmd_list_element *setlist, const char *args, int from_tty)
+with_command_1 (const char *set_cmd_prefix,
+		cmd_list_element *setlist, const char *args, int from_tty)
 {
   const char *delim = strstr (args, "--");
   const char *nested_cmd = nullptr;
@@ -225,11 +226,10 @@ with_command_1 (cmd_list_element *setlist, const char *args, int from_tty)
   if (delim == nullptr || *skip_spaces (&delim[2]) == '\0')
     nested_cmd = repeat_previous ();
 
-  const char *cmd = args;
-  cmd_list_element *set_cmd = lookup_cmd (&args, setlist, "with", 0, 1);
-
-  if (set_cmd == nullptr)
-    error (_("Unknown setting %s"), cmd);
+  cmd_list_element *set_cmd = lookup_cmd (&args, setlist, set_cmd_prefix,
+					  /*allow_unknown=*/ 0,
+					  /*ignore_help_classes=*/ 1);
+  gdb_assert (set_cmd != nullptr);
 
   if (set_cmd->var == nullptr)
     error (_("Cannot use this setting with the \"with\" command"));
@@ -308,7 +308,7 @@ with_command_completer_1 (const char *set_cmd_prefix,
 static void
 with_command (const char *args, int from_tty)
 {
-  with_command_1 (setlist, args, from_tty);
+  with_command_1 ("set ", setlist, args, from_tty);
 }
 
 /* "with" command completer.  */
diff --git a/gdb/cli/cli-cmds.h b/gdb/cli/cli-cmds.h
index 2c8e9a676c1..94e210a84eb 100644
--- a/gdb/cli/cli-cmds.h
+++ b/gdb/cli/cli-cmds.h
@@ -143,9 +143,11 @@ extern int source_verbose;
 extern int trace_commands;
 
 /* Common code for the "with" and "maintenance with" commands.
-   SETLIST is the command list for corresponding "set" command: i.e.,
-   "set" or "maintenance set".  */
-extern void with_command_1 (cmd_list_element *setlist,
+   SET_CMD_PREFIX is the spelling of the corresponding "set" command
+   prefix: i.e., "set " or "maintenance set ".  SETLIST is the command
+   element for the same "set" command prefix.  */
+extern void with_command_1 (const char *set_cmd_prefix,
+			    cmd_list_element *setlist,
 			    const char *args, int from_tty);
 
 /* Common code for the completers of the "with" and "maintenance with"
diff --git a/gdb/maint.c b/gdb/maint.c
index 10bb4fe7e9a..2c10903539b 100644
--- a/gdb/maint.c
+++ b/gdb/maint.c
@@ -640,7 +640,7 @@ maintenance_show_cmd (const char *args, int from_tty)
 static void
 maintenance_with_cmd (const char *args, int from_tty)
 {
-  with_command_1 (maintenance_set_cmdlist, args, from_tty);
+  with_command_1 ("maintenance set ", maintenance_set_cmdlist, args, from_tty);
 }
 
 /* "maintenance with" command completer.  */
diff --git a/gdb/testsuite/gdb.base/with.exp b/gdb/testsuite/gdb.base/with.exp
index b1d3adb3fda..9ea768563a3 100644
--- a/gdb/testsuite/gdb.base/with.exp
+++ b/gdb/testsuite/gdb.base/with.exp
@@ -220,6 +220,24 @@ with_test_prefix "run control" {
 
 # Check errors.
 with_test_prefix "errors" {
+    # Try both an unknown root setting and an unknown prefixed
+    # setting.  The errors come from different locations in the
+    # sources.
+    gdb_test "with xxxx yyyy" \
+	"Undefined set command: \"xxxx\".  Try \"help set\"\\."
+    gdb_test "with print xxxx yyyy" \
+	"Undefined set print command: \"xxxx yyyy\".  Try \"help set print\"\\."
+    # Try one error case for "maint with", to make sure the right
+    # "maintenance with" prefix is shown.
+    gdb_test "maint with xxxx yyyy" \
+	"Undefined maintenance set command: \"xxxx\".  Try \"help maintenance set\"\\."
+
+    # Try ambiguous settings.
+    gdb_test "with w" \
+	"Ambiguous set command \"w\": watchdog, width, write\\."
+    gdb_test "with print m" \
+	"Ambiguous set print command \"m\": max-depth, max-symbolic-offset\\."
+
     gdb_test "with variable xxx=1" \
 	"Cannot use this setting with the \"with\" command"
 
-- 
2.14.5