[PATCH 0/4] Improvements related to deprecated command aliases

[PATCH 0/4] Improvements related to deprecated command aliases

[Andrew Burgess]

The following patches all improve how GDB handles deprecated command
aliases in various situations, and address the following issues:

  - deprecated warning appearing during tab completion,
  - deprecated warning not appearing at all in some cases,
  - deprecated warning not being i18n friendly, and
  - deprecated warning missing the alias prefix

--

Andrew Burgess (4):
  gdb: don't warn about deprecated aliases during tab completion
  gdb: give deprecated command warning for aliases with a prefix
  gdb: make deprecated_cmd_warning i18n friendly
  gdb: improve the warning given for deprecated aliases with a prefix

 gdb/ChangeLog                         |  40 ++++
 gdb/cli/cli-decode.c                  | 253 +++++++++++++-------------
 gdb/command.h                         |  54 +++++-
 gdb/completer.c                       |   5 +-
 gdb/testsuite/ChangeLog               |  13 ++
 gdb/testsuite/gdb.base/commands.exp   |  19 ++
 gdb/testsuite/gdb.base/completion.exp |  13 ++
 gdb/top.c                             |   2 +-
 8 files changed, 258 insertions(+), 141 deletions(-)

-- 
2.25.4

[PATCH 1/4] gdb: don't warn about deprecated aliases during tab completion

[Andrew Burgess]

Consider this gdb session, where on line #3 tab completion is used:

  (gdb) alias xxx_yyy_zzz=break
  (gdb) maint deprecate xxx_yyy_zzz
  (gdb) xxx_yyy_<TAB>

The third line then updates to look like this:

  (gdb) xxx_yyy_Warning: 'xxx_yyy_zzz', an alias for the command 'break' is deprecated.
  No alternative known.

  zzz

What's happened is during tab completion the alias has been resolved
to the actual command being aliased, and at this stage the warning is
issued.  Clearly this is not what we want during tab completion.

In this commit I add a new parameter to the lookup function, a boolean
that indicates if the lookup is being done as part of completion.
This flag is used to suppress the warning.  Now we get the expected
behaviour, the alias completes without any warning, but the warning is
still given once the user executes the alias.

gdb/ChangeLog:

	* cli/cli-decode.c (lookup_cmd_1): Move header comment into
	command.h, add extra parameter, and use this to guard giving a
	warning.
	* command.h (lookup_cmd_1): Add comment from cli/cli-decode.c,
	include argument names in declaration, add new argument.
	* completer.c (complete_line_internal_1): Remove unneeded
	brackets, pass extra argument to lookup_cmd_1.

gdb/testsuite/ChangeLog:

	* gdb.base/completion.exp: Add additional tests.
---
 gdb/ChangeLog                         | 10 ++++++
 gdb/cli/cli-decode.c                  | 48 +++----------------------
 gdb/command.h                         | 52 ++++++++++++++++++++++++---
 gdb/completer.c                       |  5 ++-
 gdb/testsuite/ChangeLog               |  4 +++
 gdb/testsuite/gdb.base/completion.exp |  6 ++++
 6 files changed, 74 insertions(+), 51 deletions(-)

diff --git a/gdb/cli/cli-decode.c b/gdb/cli/cli-decode.c
index 71924c3fb8c..c62b8498060 100644
--- a/gdb/cli/cli-decode.c
+++ b/gdb/cli/cli-decode.c
@@ -1613,50 +1613,12 @@ valid_user_defined_cmd_name_p (const char *name)
   return true;
 }
 
-/* This routine takes a line of TEXT and a CLIST in which to start the
-   lookup.  When it returns it will have incremented the text pointer past
-   the section of text it matched, set *RESULT_LIST to point to the list in
-   which the last word was matched, and will return a pointer to the cmd
-   list element which the text matches.  It will return NULL if no match at
-   all was possible.  It will return -1 (cast appropriately, ick) if ambigous
-   matches are possible; in this case *RESULT_LIST will be set to point to
-   the list in which there are ambiguous choices (and *TEXT will be set to
-   the ambiguous text string).
-
-   if DEFAULT_ARGS is not null, *DEFAULT_ARGS is set to the found command
-   default args (possibly empty).
-
-   If the located command was an abbreviation, this routine returns the base
-   command of the abbreviation.  Note that *DEFAULT_ARGS will contain the
-   default args defined for the alias.
-
-   It does no error reporting whatsoever; control will always return
-   to the superior routine.
-
-   In the case of an ambiguous return (-1), *RESULT_LIST will be set to point
-   at the prefix_command (ie. the best match) *or* (special case) will be NULL
-   if no prefix command was ever found.  For example, in the case of "info a",
-   "info" matches without ambiguity, but "a" could be "args" or "address", so
-   *RESULT_LIST is set to the cmd_list_element for "info".  So in this case
-   RESULT_LIST should not be interpreted as a pointer to the beginning of a
-   list; it simply points to a specific command.  In the case of an ambiguous
-   return *TEXT is advanced past the last non-ambiguous prefix (e.g.
-   "info t" can be "info types" or "info target"; upon return *TEXT has been
-   advanced past "info ").
-
-   If RESULT_LIST is NULL, don't set *RESULT_LIST (but don't otherwise
-   affect the operation).
-
-   This routine does *not* modify the text pointed to by TEXT.
-
-   If IGNORE_HELP_CLASSES is nonzero, ignore any command list elements which
-   are actually help classes rather than commands (i.e. the function field of
-   the struct cmd_list_element is NULL).  */
+/* See command.h.  */
 
 struct cmd_list_element *
 lookup_cmd_1 (const char **text, struct cmd_list_element *clist,
 	      struct cmd_list_element **result_list, std::string *default_args,
-	      int ignore_help_classes)
+	      int ignore_help_classes, bool lookup_for_completion_p)
 {
   char *command;
   int len, nfound;
@@ -1715,7 +1677,7 @@ lookup_cmd_1 (const char **text, struct cmd_list_element *clist,
        itself and we will adjust the appropriate DEPRECATED_WARN_USER
        flags.  */
 
-      if (found->deprecated_warn_user)
+      if (found->deprecated_warn_user && !lookup_for_completion_p)
 	deprecated_cmd_warning (line);
 
       /* Return the default_args of the alias, not the default_args
@@ -1729,8 +1691,8 @@ lookup_cmd_1 (const char **text, struct cmd_list_element *clist,
 
   if (found->prefixlist)
     {
-      c = lookup_cmd_1 (text, *found->prefixlist, result_list,
-			default_args, ignore_help_classes);
+      c = lookup_cmd_1 (text, *found->prefixlist, result_list, default_args,
+			ignore_help_classes, lookup_for_completion_p);
       if (!c)
 	{
 	  /* Didn't find anything; this is as far as we got.  */
diff --git a/gdb/command.h b/gdb/command.h
index 22e43de3c15..a6ddaa24905 100644
--- a/gdb/command.h
+++ b/gdb/command.h
@@ -278,11 +278,53 @@ extern struct cmd_list_element *lookup_cmd (const char **,
 					    std::string *,
 					    int, int);
 
-extern struct cmd_list_element *lookup_cmd_1 (const char **,
-					      struct cmd_list_element *,
-					      struct cmd_list_element **,
-					      std::string *,
-					      int);
+/* This routine takes a line of TEXT and a CLIST in which to start the
+   lookup.  When it returns it will have incremented the text pointer past
+   the section of text it matched, set *RESULT_LIST to point to the list in
+   which the last word was matched, and will return a pointer to the cmd
+   list element which the text matches.  It will return NULL if no match at
+   all was possible.  It will return -1 (cast appropriately, ick) if ambigous
+   matches are possible; in this case *RESULT_LIST will be set to point to
+   the list in which there are ambiguous choices (and *TEXT will be set to
+   the ambiguous text string).
+
+   if DEFAULT_ARGS is not null, *DEFAULT_ARGS is set to the found command
+   default args (possibly empty).
+
+   If the located command was an abbreviation, this routine returns the base
+   command of the abbreviation.  Note that *DEFAULT_ARGS will contain the
+   default args defined for the alias.
+
+   It does no error reporting whatsoever; control will always return
+   to the superior routine.
+
+   In the case of an ambiguous return (-1), *RESULT_LIST will be set to point
+   at the prefix_command (ie. the best match) *or* (special case) will be NULL
+   if no prefix command was ever found.  For example, in the case of "info a",
+   "info" matches without ambiguity, but "a" could be "args" or "address", so
+   *RESULT_LIST is set to the cmd_list_element for "info".  So in this case
+   RESULT_LIST should not be interpreted as a pointer to the beginning of a
+   list; it simply points to a specific command.  In the case of an ambiguous
+   return *TEXT is advanced past the last non-ambiguous prefix (e.g.
+   "info t" can be "info types" or "info target"; upon return *TEXT has been
+   advanced past "info ").
+
+   If RESULT_LIST is NULL, don't set *RESULT_LIST (but don't otherwise
+   affect the operation).
+
+   This routine does *not* modify the text pointed to by TEXT.
+
+   If IGNORE_HELP_CLASSES is nonzero, ignore any command list elements which
+   are actually help classes rather than commands (i.e. the function field of
+   the struct cmd_list_element is NULL).
+
+   When LOOKUP_FOR_COMPLETION_P is true the completion is being requested
+   for the completion engine, no warnings should be printed.  */
+
+extern struct cmd_list_element *lookup_cmd_1
+	(const char **text, struct cmd_list_element *clist,
+	 struct cmd_list_element **result_list, std::string *default_args,
+	 int ignore_help_classes, bool lookup_for_completion_p = false);
 
 extern struct cmd_list_element *deprecate_cmd (struct cmd_list_element *,
 					       const char * );
diff --git a/gdb/completer.c b/gdb/completer.c
index 7f63ced93d8..4c1ad254b07 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -1388,9 +1388,8 @@ complete_line_internal_1 (completion_tracker &tracker,
       result_list = 0;
     }
   else
-    {
-      c = lookup_cmd_1 (&p, cmdlist, &result_list, NULL, ignore_help_classes);
-    }
+    c = lookup_cmd_1 (&p, cmdlist, &result_list, NULL, ignore_help_classes,
+		      true);
 
   /* Move p up to the next interesting thing.  */
   while (*p == ' ' || *p == '\t')
diff --git a/gdb/testsuite/gdb.base/completion.exp b/gdb/testsuite/gdb.base/completion.exp
index 1c5d03b217c..14d56a5cc19 100644
--- a/gdb/testsuite/gdb.base/completion.exp
+++ b/gdb/testsuite/gdb.base/completion.exp
@@ -962,3 +962,9 @@ foreach_with_prefix cmd { "watch" "awatch" "rwatch" } {
 	    }
     }
 }
+
+# Check that tab completion of a deprecated alias does not display the
+# warning about the alias being deprecated during tab completion.
+gdb_test_no_output "alias xxx_yyy_zzz=break"
+gdb_test_no_output "maint deprecate xxx_yyy_zzz"
+test_gdb_complete_unique "xxx_yyy_" "xxx_yyy_zzz"
-- 
2.25.4

[PATCH 2/4] gdb: give deprecated command warning for aliases with a prefix

[Andrew Burgess]

I noticed that deprecated aliases that have a prefix don't give a
deprecated command warning.  For example looking in mi/mi-main.c we
see this:

  c = add_alias_cmd ("target-async", "mi-async", class_run, 0, &setlist);
  deprecate_cmd (c, "set mi-async");
  c = add_alias_cmd ("target-async", "mi-async", class_run, 0, &showlist);
  deprecate_cmd (c, "show mi-async");

So both 'set target-async' and 'show target-async' are deprecated and
should be giving a warning, however, in use we see no warning given.

This is a consequence of how the code that should give this
warning (deprecated_cmd_warning) performs a second command lookup in
order to distinguish between aliases and real commands, and that the
code that calls this (lookup_cmd_1) strips off prefix commands as it
calls itself recursively.

As a result when we are considering an alias like 'set target-async'
we first enter lookup_cmd_1 with text = "set target-async", we spot
the 'set' command prefix and then recursively call lookup_cmd_1 with
text = "target-async".

We spot that 'target-async' is a known alias but that it is
deprecated, and so call deprecated_cmd_warning passing in the value of
text, which remember is now "target-async".

In deprecated_cmd_warning we again perform a command lookup starting
from the top-level cmdlist, but now we're trying to find just
"target-async", this fails (as this command requires the 'set' prefix,
and so no warning is given.

I resolved this issue by passing a command list to the function
deprecated_cmd_warning, this is the list in which the command can be
found.

A new test is added to cover this case.

However, there is an additional problem which will be addressed in a
subsequent patch.

Consider this GDB session:

  (gdb) define set xxx_yyy
  Type commands for definition of "set xxx_yyy".
  End with a line saying just "end".
  >echo in set xxx_yyy command\n
  >end
  (gdb) alias set qqq_aaa=set xxx_yyy
  (gdb) maintenance deprecate set qqq_aaa
  (gdb) set qqq_aaa
  Warning: 'qqq_aaa', an alias for the command 'xxx_yyy' is deprecated.
  No alternative known.

  in set xxx_yyy command
  (gdb)

Notice the warning mentions 'qqq_aaa' and 'xxx_yyy', I consider this
to be wrong.  I think the proper warning should read:

  (gdb) set qqq_aaa
  Warning: 'set qqq_aaa', an alias for the command 'set xxx_yyy' is deprecated.
  No alternative known.

With the 'set' prefixes added.  A later patch will resolve this
issue.

gdb/ChangeLog:

	* cli/cli-decode.c (lookup_cmd_1): Pass command list to
	deprecated_cmd_warning.
	(deprecated_cmd_warning): Take extra parameter, call
	lookup_cmd_composition_1 and pass new parameter through.
	(lookup_cmd_composition_1): New function, takes implementation of
	lookup_cmd_composition but with extra parameter.
	(lookup_cmd_composition): Now calls lookup_cmd_composition_1
	passing in cmdlist.
	* command.h (deprecated_cmd_warning): Add extra parameter to
	declaration.
	* top.c (execute_command): Pass cmdlist to deprecated_cmd_warning.

gdb/testsuite/ChangeLog:

	* gdb.base/commands.exp: Add additional tests.
	* gdb.base/completion.exp: Add additional tests.
---
 gdb/ChangeLog                         | 14 +++++++
 gdb/cli/cli-decode.c                  | 54 ++++++++++++++++++++-------
 gdb/command.h                         |  2 +-
 gdb/testsuite/ChangeLog               |  5 +++
 gdb/testsuite/gdb.base/commands.exp   | 19 ++++++++++
 gdb/testsuite/gdb.base/completion.exp |  7 ++++
 gdb/top.c                             |  2 +-
 7 files changed, 88 insertions(+), 15 deletions(-)

diff --git a/gdb/cli/cli-decode.c b/gdb/cli/cli-decode.c
index c62b8498060..dc83f5560e6 100644
--- a/gdb/cli/cli-decode.c
+++ b/gdb/cli/cli-decode.c
@@ -50,6 +50,12 @@ static void help_cmd_list (struct cmd_list_element *list,
 
 static void help_all (struct ui_file *stream);
 
+static int lookup_cmd_composition_1 (const char *text,
+				     struct cmd_list_element **alias,
+				     struct cmd_list_element **prefix_cmd,
+				     struct cmd_list_element **cmd,
+				     struct cmd_list_element *cur_list);
+
 /* Look up a command whose 'prefixlist' is KEY.  Return the command if found,
    otherwise return NULL.  */
 
@@ -1678,7 +1684,7 @@ lookup_cmd_1 (const char **text, struct cmd_list_element *clist,
        flags.  */
 
       if (found->deprecated_warn_user && !lookup_for_completion_p)
-	deprecated_cmd_warning (line);
+	deprecated_cmd_warning (line, clist);
 
       /* Return the default_args of the alias, not the default_args
 	 of the command it is pointing to.  */
@@ -1891,13 +1897,13 @@ lookup_cmd (const char **line, struct cmd_list_element *list,
    
 */
 void
-deprecated_cmd_warning (const char *text)
+deprecated_cmd_warning (const char *text, struct cmd_list_element *list)
 {
   struct cmd_list_element *alias = NULL;
   struct cmd_list_element *prefix_cmd = NULL;
   struct cmd_list_element *cmd = NULL;
 
-  if (!lookup_cmd_composition (text, &alias, &prefix_cmd, &cmd))
+  if (!lookup_cmd_composition_1 (text, &alias, &prefix_cmd, &cmd, list))
     /* Return if text doesn't evaluate to a command.  */
     return;
 
@@ -1949,8 +1955,7 @@ deprecated_cmd_warning (const char *text)
   cmd->deprecated_warn_user = 0;
 }
 
-
-/* Look up the contents of TEXT as a command in the command list 'cmdlist'.
+/* Look up the contents of TEXT as a command in the command list CUR_LIST.
    Return 1 on success, 0 on failure.
 
    If TEXT refers to an alias, *ALIAS will point to that alias.
@@ -1964,23 +1969,22 @@ deprecated_cmd_warning (const char *text)
    exist, they are NULL when we return.
 
 */
-int
-lookup_cmd_composition (const char *text,
-			struct cmd_list_element **alias,
-			struct cmd_list_element **prefix_cmd,
-			struct cmd_list_element **cmd)
+
+static int
+lookup_cmd_composition_1 (const char *text,
+			  struct cmd_list_element **alias,
+			  struct cmd_list_element **prefix_cmd,
+			  struct cmd_list_element **cmd,
+			  struct cmd_list_element *cur_list)
 {
   char *command;
   int len, nfound;
-  struct cmd_list_element *cur_list;
   struct cmd_list_element *prev_cmd;
 
   *alias = NULL;
   *prefix_cmd = NULL;
   *cmd = NULL;
 
-  cur_list = cmdlist;
-
   text = skip_spaces (text);
 
   while (1)
@@ -2038,6 +2042,30 @@ lookup_cmd_composition (const char *text,
     }
 }
 
+/* Look up the contents of TEXT as a command in the command list 'cmdlist'.
+   Return 1 on success, 0 on failure.
+
+   If TEXT refers to an alias, *ALIAS will point to that alias.
+
+   If TEXT is a subcommand (i.e. one that is preceded by a prefix
+   command) set *PREFIX_CMD.
+
+   Set *CMD to point to the command TEXT indicates.
+
+   If any of *ALIAS, *PREFIX_CMD, or *CMD cannot be determined or do not
+   exist, they are NULL when we return.
+
+*/
+
+int
+lookup_cmd_composition (const char *text,
+			struct cmd_list_element **alias,
+			struct cmd_list_element **prefix_cmd,
+			struct cmd_list_element **cmd)
+{
+  return lookup_cmd_composition_1 (text, alias, prefix_cmd, cmd, cmdlist);
+}
+
 /* Helper function for SYMBOL_COMPLETION_FUNCTION.  */
 
 /* Return a vector of char pointers which point to the different
diff --git a/gdb/command.h b/gdb/command.h
index a6ddaa24905..9e61d19d0bd 100644
--- a/gdb/command.h
+++ b/gdb/command.h
@@ -329,7 +329,7 @@ extern struct cmd_list_element *lookup_cmd_1
 extern struct cmd_list_element *deprecate_cmd (struct cmd_list_element *,
 					       const char * );
 
-extern void deprecated_cmd_warning (const char *);
+extern void deprecated_cmd_warning (const char *, struct cmd_list_element *);
 
 extern int lookup_cmd_composition (const char *text,
 				   struct cmd_list_element **alias,
diff --git a/gdb/testsuite/gdb.base/commands.exp b/gdb/testsuite/gdb.base/commands.exp
index 7ff57f774af..19f712c46f3 100644
--- a/gdb/testsuite/gdb.base/commands.exp
+++ b/gdb/testsuite/gdb.base/commands.exp
@@ -689,6 +689,25 @@ proc_with_prefix deprecated_command_test {} {
     gdb_test "maintenance deprecate" \
 	    "\"maintenance deprecate\".*" \
 	    "deprecate with no arguments"
+
+    # Test that an alias with a prefix still gives a warning.
+    set file1 [standard_output_file xxx_yyy_cmd]
+    set fd [open "$file1" w]
+    puts $fd \
+"define set xxx_yyy
+echo in command xxx_yyy\\n
+end
+
+alias set qqq_aaa=set xxx_yyy
+maintenance deprecate set qqq_aaa"
+    close $fd
+    gdb_test_no_output "source $file1" \
+	"source file containing xxx_yyy command and its alias"
+    gdb_test "set qqq_aaa" \
+	"Warning: 'qqq_aaa', an alias for the command 'xxx_yyy' is deprecated\\.\r\n.*No alternative known\\..*" \
+	"deprecated alias with prefix give a warning"
+
+    file delete $file1
 }
 
 proc_with_prefix bp_deleted_in_command_test {} {
diff --git a/gdb/testsuite/gdb.base/completion.exp b/gdb/testsuite/gdb.base/completion.exp
index 14d56a5cc19..8000d520491 100644
--- a/gdb/testsuite/gdb.base/completion.exp
+++ b/gdb/testsuite/gdb.base/completion.exp
@@ -968,3 +968,10 @@ foreach_with_prefix cmd { "watch" "awatch" "rwatch" } {
 gdb_test_no_output "alias xxx_yyy_zzz=break"
 gdb_test_no_output "maint deprecate xxx_yyy_zzz"
 test_gdb_complete_unique "xxx_yyy_" "xxx_yyy_zzz"
+
+# Check that tab completion of a deprecated alias with a prefix does
+# not display the warning about the alias being deprecated during tab
+# completion.
+gdb_test_no_output "alias set aaa_bbb_ccc=set debug"
+gdb_test_no_output "maint deprecate set aaa_bbb_ccc"
+test_gdb_complete_unique "set aaa_bbb_" "set aaa_bbb_ccc"
diff --git a/gdb/top.c b/gdb/top.c
index e82828e48d7..b515bbfbd19 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -639,7 +639,7 @@ execute_command (const char *p, int from_tty)
       execute_cmd_pre_hook (c);
 
       if (c->deprecated_warn_user)
-	deprecated_cmd_warning (line);
+	deprecated_cmd_warning (line, cmdlist);
 
       /* c->user_commands would be NULL in the case of a python command.  */
       if (c->theclass == class_user && c->user_commands)
-- 
2.25.4

[PATCH 3/4] gdb: make deprecated_cmd_warning i18n friendly

[Andrew Burgess]

Rewrite deprecated_cmd_warning to be i18n friendly.  While I'm going
through the function I also cleaned up some whitespace issues,
replaced uses of NULL with nullptr, and moved some comments to avoid
having to add { ... }.

Though the message being printed has a 'Warning: ' prefix I could have
changed from using printf_filtered to use warning, however, I haven't
done that in this commit as that would change what GDB outputs and I
wanted this commit NOT to change the output.

There should be no user visible changes after this commit.

gdb/ChangeLog:

	* cli/cli-decode.c (deprecated_cmd_warning): Use nullptr instead
	of NULL.  Don't print message piece by piece, but sentence at a
	time to allow internationalisation.  Some whitespace cleanup.
---
 gdb/ChangeLog        |  6 ++++
 gdb/cli/cli-decode.c | 79 ++++++++++++++++++++++----------------------
 2 files changed, 45 insertions(+), 40 deletions(-)

diff --git a/gdb/cli/cli-decode.c b/gdb/cli/cli-decode.c
index dc83f5560e6..2ad7717fbc6 100644
--- a/gdb/cli/cli-decode.c
+++ b/gdb/cli/cli-decode.c
@@ -1686,6 +1686,7 @@ lookup_cmd_1 (const char **text, struct cmd_list_element *clist,
       if (found->deprecated_warn_user && !lookup_for_completion_p)
 	deprecated_cmd_warning (line, clist);
 
+
       /* Return the default_args of the alias, not the default_args
 	 of the command it is pointing to.  */
       if (default_args != nullptr)
@@ -1899,59 +1900,57 @@ lookup_cmd (const char **line, struct cmd_list_element *list,
 void
 deprecated_cmd_warning (const char *text, struct cmd_list_element *list)
 {
-  struct cmd_list_element *alias = NULL;
-  struct cmd_list_element *prefix_cmd = NULL;
-  struct cmd_list_element *cmd = NULL;
+  struct cmd_list_element *alias = nullptr;
+  struct cmd_list_element *prefix_cmd = nullptr;
+  struct cmd_list_element *cmd = nullptr;
 
+  /* Return if text doesn't evaluate to a command.  */
   if (!lookup_cmd_composition_1 (text, &alias, &prefix_cmd, &cmd, list))
-    /* Return if text doesn't evaluate to a command.  */
     return;
 
-  if (!((alias ? alias->deprecated_warn_user : 0)
-      || cmd->deprecated_warn_user) ) 
-    /* Return if nothing is deprecated.  */
+  /* Return if nothing is deprecated.  */
+  if (!((alias != nullptr ? alias->deprecated_warn_user : 0)
+	|| cmd->deprecated_warn_user))
     return;
-  
-  printf_filtered ("Warning:");
-  
-  if (alias && !cmd->cmd_deprecated)
-    printf_filtered (" '%s', an alias for the", alias->name);
-    
-  printf_filtered (" command '");
-  
-  if (prefix_cmd)
-    printf_filtered ("%s", prefix_cmd->prefixname);
-  
-  printf_filtered ("%s", cmd->name);
-
-  if (alias && cmd->cmd_deprecated)
-    printf_filtered ("' (%s) is deprecated.\n", alias->name);
-  else
-    printf_filtered ("' is deprecated.\n"); 
-  
 
-  /* If it is only the alias that is deprecated, we want to indicate
-     the new alias, otherwise we'll indicate the new command.  */
+  /* Join command prefix (if any) and the command name.  */
+  std::string tmp_cmd_str;
+  if (prefix_cmd != nullptr)
+    tmp_cmd_str += std::string (prefix_cmd->prefixname);
+  tmp_cmd_str += std::string (cmd->name);
 
-  if (alias && !cmd->cmd_deprecated)
-    {
-      if (alias->replacement)
-	printf_filtered ("Use '%s'.\n\n", alias->replacement);
-      else
-	printf_filtered ("No alternative known.\n\n");
-     }  
-  else
+  /* Display the appropriate first line, this warns that the thing the user
+     entered is deprecated.  */
+  if (alias != nullptr)
     {
-      if (cmd->replacement)
-	printf_filtered ("Use '%s'.\n\n", cmd->replacement);
+      if (cmd->cmd_deprecated)
+	printf_filtered (_("Warning: command '%s' (%s) is deprecated.\n"),
+			 tmp_cmd_str.c_str (), alias->name);
       else
-	printf_filtered ("No alternative known.\n\n");
+	printf_filtered (_("Warning: '%s', an alias for the command '%s' "
+			   "is deprecated.\n"),
+			 alias->name, tmp_cmd_str.c_str ());
     }
+  else
+    printf_filtered (_("Warning: command '%s' is deprecated.\n"),
+		     tmp_cmd_str.c_str ());
+
+  /* Now display a second line indicating what the user should use instead.
+     If it is only the alias that is deprecated, we want to indicate the
+     new alias, otherwise we'll indicate the new command.  */
+  const char *replacement;
+  if (alias != nullptr && !cmd->cmd_deprecated)
+    replacement = alias->replacement;
+  else
+    replacement = cmd->replacement;
+  if (replacement != nullptr)
+    printf_filtered (_("Use '%s'.\n\n"), replacement);
+  else
+    printf_filtered (_("No alternative known.\n\n"));
 
   /* We've warned you, now we'll keep quiet.  */
-  if (alias)
+  if (alias != nullptr)
     alias->deprecated_warn_user = 0;
-  
   cmd->deprecated_warn_user = 0;
 }
 
-- 
2.25.4

[PATCH 4/4] gdb: improve the warning given for deprecated aliases with a prefix

[Andrew Burgess]

Consider this GDB session:

  (gdb) define set xxx_yyy
  Type commands for definition of "set xxx_yyy".
  End with a line saying just "end".
  >echo in set xxx_yyy command\n
  >end
  (gdb) alias set qqq_aaa=set xxx_yyy
  (gdb) maintenance deprecate set qqq_aaa
  (gdb) set qqq_aaa
  Warning: 'qqq_aaa', an alias for the command 'xxx_yyy' is deprecated.
  No alternative known.

  in set xxx_yyy command
  (gdb)

Notice the warning mentions 'qqq_aaa' and 'xxx_yyy', I consider this
to be wrong.  I think the proper warning should read:

  (gdb) set qqq_aaa
  Warning: 'set qqq_aaa', an alias for the command 'set xxx_yyy' is deprecated.
  No alternative known.

With the 'set' prefixes added.  That is what this patch does.  The
expected results from one test needed to be updated.

gdb/ChangeLog:

	* cli/cli-decode.c (deprecated_cmd_warning): Ignore the prefix
	result from lookup_cmd_composition_1, use the prefixes from both
	the command and the alias instead.
	(lookup_cmd_composition_1): Initial prefix command is the based on
	the search list being passed in.  Simplify the logic for tracking
	the prefix command.  Replace a use of alloca with a local
	std::string.

gdb/testsuite/ChangeLog:

	* gdb.base/commands.exp: Update expected results.
---
 gdb/ChangeLog                       | 10 ++++
 gdb/cli/cli-decode.c                | 82 +++++++++++++++--------------
 gdb/testsuite/ChangeLog             |  4 ++
 gdb/testsuite/gdb.base/commands.exp |  2 +-
 4 files changed, 57 insertions(+), 41 deletions(-)

diff --git a/gdb/cli/cli-decode.c b/gdb/cli/cli-decode.c
index 2ad7717fbc6..b77dd711779 100644
--- a/gdb/cli/cli-decode.c
+++ b/gdb/cli/cli-decode.c
@@ -1901,12 +1901,21 @@ void
 deprecated_cmd_warning (const char *text, struct cmd_list_element *list)
 {
   struct cmd_list_element *alias = nullptr;
-  struct cmd_list_element *prefix_cmd = nullptr;
   struct cmd_list_element *cmd = nullptr;
 
-  /* Return if text doesn't evaluate to a command.  */
-  if (!lookup_cmd_composition_1 (text, &alias, &prefix_cmd, &cmd, list))
-    return;
+  /* Return if text doesn't evaluate to a command.  We place this lookup
+     within its own scope so that the PREFIX_CMD local is not visible
+     later in this function.  The value returned in PREFIX_CMD is based on
+     the prefix found in TEXT, and is our case this prefix can be missing
+     in some situations (when LIST is not the global CMDLIST).
+
+     It is better for our purposes to use the prefix commands directly from
+     the ALIAS and CMD results.  */
+  {
+    struct cmd_list_element *prefix_cmd = nullptr;
+    if (!lookup_cmd_composition_1 (text, &alias, &prefix_cmd, &cmd, list))
+      return;
+  }
 
   /* Return if nothing is deprecated.  */
   if (!((alias != nullptr ? alias->deprecated_warn_user : 0)
@@ -1915,21 +1924,27 @@ deprecated_cmd_warning (const char *text, struct cmd_list_element *list)
 
   /* Join command prefix (if any) and the command name.  */
   std::string tmp_cmd_str;
-  if (prefix_cmd != nullptr)
-    tmp_cmd_str += std::string (prefix_cmd->prefixname);
+  if (cmd->prefix != nullptr)
+    tmp_cmd_str += std::string (cmd->prefix->prefixname);
   tmp_cmd_str += std::string (cmd->name);
 
   /* Display the appropriate first line, this warns that the thing the user
      entered is deprecated.  */
   if (alias != nullptr)
     {
+      /* Join the alias prefix (if any) and the alias name.  */
+      std::string tmp_alias_str;
+      if (alias->prefix != nullptr)
+	tmp_alias_str += std::string (alias->prefix->prefixname);
+      tmp_alias_str += std::string (alias->name);
+
       if (cmd->cmd_deprecated)
 	printf_filtered (_("Warning: command '%s' (%s) is deprecated.\n"),
-			 tmp_cmd_str.c_str (), alias->name);
+			 tmp_cmd_str.c_str (), tmp_alias_str.c_str ());
       else
 	printf_filtered (_("Warning: '%s', an alias for the command '%s' "
 			   "is deprecated.\n"),
-			 alias->name, tmp_cmd_str.c_str ());
+			 tmp_alias_str.c_str (), tmp_cmd_str.c_str ());
     }
   else
     printf_filtered (_("Warning: command '%s' is deprecated.\n"),
@@ -1976,25 +1991,18 @@ lookup_cmd_composition_1 (const char *text,
 			  struct cmd_list_element **cmd,
 			  struct cmd_list_element *cur_list)
 {
-  char *command;
-  int len, nfound;
-  struct cmd_list_element *prev_cmd;
-
-  *alias = NULL;
-  *prefix_cmd = NULL;
-  *cmd = NULL;
+  *alias = nullptr;
+  *prefix_cmd = cur_list->prefix;
+  *cmd = nullptr;
 
   text = skip_spaces (text);
 
+  /* Go through as many command lists as we need to, to find the command
+     TEXT refers to.  */
   while (1)
     {
-      /* Go through as many command lists as we need to,
-	 to find the command TEXT refers to.  */
-
-      prev_cmd = *cmd;
-
       /* Identify the name of the command.  */
-      len = find_command_name_length (text);
+      int len = find_command_name_length (text);
 
       /* If nothing but whitespace, return.  */
       if (len == 0)
@@ -2002,40 +2010,34 @@ lookup_cmd_composition_1 (const char *text,
 
       /* TEXT is the start of the first command word to lookup (and
 	 it's length is LEN).  We copy this into a local temporary.  */
-
-      command = (char *) alloca (len + 1);
-      memcpy (command, text, len);
-      command[len] = '\0';
+      std::string command (text, len);
 
       /* Look it up.  */
-      *cmd = 0;
-      nfound = 0;
-      *cmd = find_cmd (command, len, cur_list, 1, &nfound);
-
-      if (*cmd == CMD_LIST_AMBIGUOUS)
-	{
-	  return 0;              /* ambiguous */
-	}
+      int nfound = 0;
+      *cmd = find_cmd (command.c_str (), len, cur_list, 1, &nfound);
 
-      if (*cmd == NULL)
-	return 0;                /* nothing found */
+      /* We only handle the case where a single command was found.  */
+      if (*cmd == CMD_LIST_AMBIGUOUS || *cmd == nullptr)
+	return 0;
       else
 	{
 	  if ((*cmd)->cmd_pointer)
 	    {
-	      /* cmd was actually an alias, we note that an alias was
-		 used (by assigning *ALIAS) and we set *CMD.  */
+	      /* If the command was actually an alias, we note that an
+		 alias was used (by assigning *ALIAS) and we set *CMD.  */
 	      *alias = *cmd;
 	      *cmd = (*cmd)->cmd_pointer;
 	    }
-	  *prefix_cmd = prev_cmd;
 	}
 
       text += len;
       text = skip_spaces (text);
 
-      if ((*cmd)->prefixlist && *text != '\0')
-	cur_list = *(*cmd)->prefixlist;
+      if ((*cmd)->prefixlist != nullptr && *text != '\0')
+	{
+	  cur_list = *(*cmd)->prefixlist;
+	  *prefix_cmd = *cmd;
+	}
       else
 	return 1;
     }
diff --git a/gdb/testsuite/gdb.base/commands.exp b/gdb/testsuite/gdb.base/commands.exp
index 19f712c46f3..bf3f6f030cb 100644
--- a/gdb/testsuite/gdb.base/commands.exp
+++ b/gdb/testsuite/gdb.base/commands.exp
@@ -704,7 +704,7 @@ maintenance deprecate set qqq_aaa"
     gdb_test_no_output "source $file1" \
 	"source file containing xxx_yyy command and its alias"
     gdb_test "set qqq_aaa" \
-	"Warning: 'qqq_aaa', an alias for the command 'xxx_yyy' is deprecated\\.\r\n.*No alternative known\\..*" \
+	"Warning: 'set qqq_aaa', an alias for the command 'set xxx_yyy' is deprecated\\.\r\n.*No alternative known\\..*" \
 	"deprecated alias with prefix give a warning"
 
     file delete $file1
-- 
2.25.4

Re: [PATCH 2/4] gdb: give deprecated command warning for aliases with a prefix

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> I noticed that deprecated aliases that have a prefix don't give a
Andrew> deprecated command warning.

This is https://sourceware.org/bugzilla/show_bug.cgi?id=15104 so I
think a note should be added to the ChangeLog here.

Tom

Re: [PATCH 3/4] gdb: make deprecated_cmd_warning i18n friendly

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> Rewrite deprecated_cmd_warning to be i18n friendly.  While I'm going
Andrew> through the function I also cleaned up some whitespace issues,
Andrew> replaced uses of NULL with nullptr, and moved some comments to avoid
Andrew> having to add { ... }.

Andrew> +	printf_filtered (_("Warning: command '%s' (%s) is deprecated.\n"),
Andrew> +			 tmp_cmd_str.c_str (), alias->name);

Thanks for doing this.  This makes it simpler to add styling here.

Tom

Re: [PATCH 4/4] gdb: improve the warning given for deprecated aliases with a prefix

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> Notice the warning mentions 'qqq_aaa' and 'xxx_yyy', I consider this
Andrew> to be wrong.  I think the proper warning should read:

Andrew>   (gdb) set qqq_aaa
Andrew>   Warning: 'set qqq_aaa', an alias for the command 'set xxx_yyy' is deprecated.

I would normally also write a comma before "is deprecated":

    Warning: 'set qqq_aaa', an alias for the command 'set xxx_yyy', is deprecated.

I'm not sure if this is grammatically correct and/or necessary though.

Tom

Re: [PATCH 0/4] Improvements related to deprecated command aliases

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> The following patches all improve how GDB handles deprecated command
Andrew> aliases in various situations, and address the following issues:

Andrew>   - deprecated warning appearing during tab completion,
Andrew>   - deprecated warning not appearing at all in some cases,
Andrew>   - deprecated warning not being i18n friendly, and
Andrew>   - deprecated warning missing the alias prefix

I read through this and everything looks good to me.  Thank you for
doing this; the CLI machinery probably should get a bit more love than
it does.

Tom