[PATCH 1/3] gdb: use intrusive_list for cmd_list_element aliases list

[PATCH 1/3] gdb: use intrusive_list for cmd_list_element aliases list

[Simon Marchi]

Change the manually-implemented linked list to use intrusive_list.  This
is not strictly necessary, but it makes the code much simpler.

Change-Id: Idd08090ebf2db8bdcf68e85ef72a9635f1584ccc
---
 gdb/cli/cli-decode.c | 82 ++++++++++++++++++++------------------------
 gdb/cli/cli-decode.h | 18 +++++++---
 2 files changed, 50 insertions(+), 50 deletions(-)

diff --git a/gdb/cli/cli-decode.c b/gdb/cli/cli-decode.c
index 030cba443386..39f9eb54dc20 100644
--- a/gdb/cli/cli-decode.c
+++ b/gdb/cli/cli-decode.c
@@ -30,12 +30,10 @@
 
 static void undef_cmd_error (const char *, const char *);
 
-static struct cmd_list_element *delete_cmd (const char *name,
-					    struct cmd_list_element **list,
-					    struct cmd_list_element **prehook,
-					    struct cmd_list_element **prehookee,
-					    struct cmd_list_element **posthook,
-					    struct cmd_list_element **posthookee);
+static cmd_list_element::aliases_list_type delete_cmd
+  (const char *name, cmd_list_element **list, cmd_list_element **prehook,
+   cmd_list_element **prehookee, cmd_list_element **posthook,
+   cmd_list_element **posthookee);
 
 static struct cmd_list_element *find_cmd (const char *command,
 					  int len,
@@ -171,20 +169,24 @@ do_add_cmd (const char *name, enum command_class theclass,
 {
   struct cmd_list_element *c = new struct cmd_list_element (name, theclass,
 							    doc);
-  struct cmd_list_element *p, *iter;
 
   /* Turn each alias of the old command into an alias of the new
      command.  */
   c->aliases = delete_cmd (name, list, &c->hook_pre, &c->hookee_pre,
 			   &c->hook_post, &c->hookee_post);
-  for (iter = c->aliases; iter; iter = iter->alias_chain)
-    iter->alias_target = c;
+
+  for (cmd_list_element &alias : c->aliases)
+    alias.alias_target = c;
+
   if (c->hook_pre)
     c->hook_pre->hookee_pre = c;
+
   if (c->hookee_pre)
     c->hookee_pre->hook_pre = c;
+
   if (c->hook_post)
     c->hook_post->hookee_post = c;
+
   if (c->hookee_post)
     c->hookee_post->hook_post = c;
 
@@ -195,7 +197,7 @@ do_add_cmd (const char *name, enum command_class theclass,
     }
   else
     {
-      p = *list;
+      cmd_list_element *p = *list;
       while (p->next && strcmp (p->next->name, name) <= 0)
 	{
 	  p = p->next;
@@ -296,8 +298,7 @@ add_alias_cmd (const char *name, cmd_list_element *target,
   c->allow_unknown = target->allow_unknown;
   c->abbrev_flag = abbrev_flag;
   c->alias_target = target;
-  c->alias_chain = target->aliases;
-  target->aliases = c;
+  target->aliases.push_front (*c);
 
   return c;
 }
@@ -1195,14 +1196,13 @@ add_setshow_zuinteger_cmd (const char *name, command_class theclass,
 					     show_list);
 }
 
-/* Remove the command named NAME from the command list.  Return the
-   list commands which were aliased to the deleted command.  If the
-   command had no aliases, return NULL.  The various *HOOKs are set to
-   the pre- and post-hook commands for the deleted command.  If the
-   command does not have a hook, the corresponding out parameter is
-   set to NULL.  */
+/* Remove the command named NAME from the command list.  Return the list
+   commands which were aliased to the deleted command.  The various *HOOKs are
+   set to the pre- and post-hook commands for the deleted command.  If the
+   command does not have a hook, the corresponding out parameter is set to
+   NULL.  */
 
-static struct cmd_list_element *
+static cmd_list_element::aliases_list_type
 delete_cmd (const char *name, struct cmd_list_element **list,
 	    struct cmd_list_element **prehook,
 	    struct cmd_list_element **prehookee,
@@ -1211,7 +1211,7 @@ delete_cmd (const char *name, struct cmd_list_element **list,
 {
   struct cmd_list_element *iter;
   struct cmd_list_element **previous_chain_ptr;
-  struct cmd_list_element *aliases = NULL;
+  cmd_list_element::aliases_list_type aliases;
 
   *prehook = NULL;
   *prehookee = NULL;
@@ -1238,21 +1238,14 @@ delete_cmd (const char *name, struct cmd_list_element **list,
 	  /* Update the link.  */
 	  *previous_chain_ptr = iter->next;
 
-	  aliases = iter->aliases;
+	  aliases = std::move (iter->aliases);
 
 	  /* If this command was an alias, remove it from the list of
 	     aliases.  */
 	  if (iter->is_alias ())
 	    {
-	      struct cmd_list_element **prevp = &iter->alias_target->aliases;
-	      struct cmd_list_element *a = *prevp;
-
-	      while (a != iter)
-		{
-		  prevp = &a->alias_chain;
-		  a = *prevp;
-		}
-	      *prevp = iter->alias_chain;
+	      auto it = iter->alias_target->aliases.iterator_to (*iter);
+	      iter->alias_target->aliases.erase (it);
 	    }
 
 	  delete iter;
@@ -1351,11 +1344,9 @@ static void
 fput_aliases_definition_styled (struct cmd_list_element *cmd,
 				struct ui_file *stream)
 {
-  for (cmd_list_element *iter = cmd->aliases;
-       iter != nullptr;
-       iter = iter->alias_chain)
-    if (!iter->default_args.empty ())
-      fput_alias_definition_styled (iter, stream);
+  for (cmd_list_element &alias : cmd->aliases)
+    if (!alias.default_args.empty ())
+      fput_alias_definition_styled (&alias, stream);
 }
 
 
@@ -1370,17 +1361,17 @@ fput_command_names_styled (struct cmd_list_element *c,
 			   bool always_fput_c_name, const char *postfix,
 			   struct ui_file *stream)
 {
-  if (always_fput_c_name ||  c->aliases != nullptr)
+  if (always_fput_c_name || !c->aliases.empty ())
     fput_command_name_styled (c, stream);
 
-  for (cmd_list_element *iter = c->aliases; iter; iter = iter->alias_chain)
+  for (cmd_list_element &alias : c->aliases)
     {
       fputs_filtered (", ", stream);
       wrap_here ("   ");
-      fput_command_name_styled (iter, stream);
+      fput_command_name_styled (&alias, stream);
     }
 
-  if (always_fput_c_name ||  c->aliases != nullptr)
+  if (always_fput_c_name || !c->aliases.empty ())
     fputs_filtered (postfix, stream);
 }
 
@@ -1453,14 +1444,15 @@ apropos_cmd (struct ui_file *stream,
 	    print_doc_of_command (c, prefix, verbose, regex, stream);
 
 	  /* Try to match against the name of the aliases.  */
-	  for (cmd_list_element *iter = c->aliases;
-	       returnvalue < 0 && iter;
-	       iter = iter->alias_chain)
+	  for (const cmd_list_element &alias : c->aliases)
 	    {
-	      name_len = strlen (iter->name);
-	      returnvalue = regex.search (iter->name, name_len, 0, name_len, NULL);
+	      name_len = strlen (alias.name);
+	      returnvalue = regex.search (alias.name, name_len, 0, name_len, NULL);
 	      if (returnvalue >= 0)
-		print_doc_of_command (c, prefix, verbose, regex, stream);
+		{
+		  print_doc_of_command (c, prefix, verbose, regex, stream);
+		  break;
+		}
 	    }
 	}
       if (c->doc != NULL && returnvalue < 0)
diff --git a/gdb/cli/cli-decode.h b/gdb/cli/cli-decode.h
index f7945ba2bf5f..1d3a3db786ef 100644
--- a/gdb/cli/cli-decode.h
+++ b/gdb/cli/cli-decode.h
@@ -26,6 +26,7 @@
 #include "gdb_regex.h"
 #include "cli-script.h"
 #include "completer.h"
+#include "gdbsupport/intrusive_list.h"
 
 /* Not a set/show command.  Note that some commands which begin with
    "set" or "show" might be in this category, if their syntax does
@@ -246,11 +247,18 @@ struct cmd_list_element
      aliased command can be located in case it has been hooked.  */
   struct cmd_list_element *alias_target = nullptr;
 
-  /* Start of a linked list of all aliases of this command.  */
-  struct cmd_list_element *aliases = nullptr;
-
-  /* Link pointer for aliases on an alias list.  */
-  struct cmd_list_element *alias_chain = nullptr;
+  /* Node to link aliases on an alias list.  */
+  using aliases_list_node_type
+    = intrusive_list_node<cmd_list_element>;
+  aliases_list_node_type aliases_list_node;
+
+  /* Linked list of all aliases of this command.  */
+  using aliases_list_member_node_type
+    = intrusive_member_node<cmd_list_element,
+			    &cmd_list_element::aliases_list_node>;
+  using aliases_list_type
+    = intrusive_list<cmd_list_element, aliases_list_member_node_type>;
+  aliases_list_type aliases;
 
   /* If non-null, the pointer to a field in 'struct
      cli_suppress_notification', which will be set to true in cmd_func
-- 
2.33.1

[PATCH 2/3] gdb: change some alias functions parameters to const-reference

[Simon Marchi]

Now that we use intrusive list to link aliases, it becomes easier to
pass cmd_list_element arguments by const-reference rather than by
pointer to some functions, change a few.

Change-Id: Id0df648ed26e9447da0671fc2c858981cda31df8
---
 gdb/cli/cli-decode.c | 64 ++++++++++++++++++++++----------------------
 1 file changed, 32 insertions(+), 32 deletions(-)

diff --git a/gdb/cli/cli-decode.c b/gdb/cli/cli-decode.c
index 39f9eb54dc20..b8be3f54921f 100644
--- a/gdb/cli/cli-decode.c
+++ b/gdb/cli/cli-decode.c
@@ -86,7 +86,7 @@ lookup_cmd_with_subcommands (cmd_list_element **subcommands,
 }
 
 static void
-print_help_for_command (struct cmd_list_element *c,
+print_help_for_command (const cmd_list_element &c,
 			bool recurse, struct ui_file *stream);
 
 static void
@@ -1314,39 +1314,39 @@ add_com_suppress_notification (const char *name, enum command_class theclass,
 /* Print the prefix of C followed by name of C in title style.  */
 
 static void
-fput_command_name_styled (struct cmd_list_element *c, struct ui_file *stream)
+fput_command_name_styled (const cmd_list_element &c, struct ui_file *stream)
 {
   std::string prefixname
-    = c->prefix == nullptr ? "" : c->prefix->prefixname ();
+    = c.prefix == nullptr ? "" : c.prefix->prefixname ();
 
   fprintf_styled (stream, title_style.style (), "%s%s",
-		  prefixname.c_str (), c->name);
+		  prefixname.c_str (), c.name);
 }
 
 /* Print the definition of alias C using title style for alias
    and aliased command.  */
 
 static void
-fput_alias_definition_styled (struct cmd_list_element *c,
+fput_alias_definition_styled (const cmd_list_element &c,
 			      struct ui_file *stream)
 {
-  gdb_assert (c->is_alias ());
+  gdb_assert (c.is_alias ());
   fputs_filtered ("  alias ", stream);
   fput_command_name_styled (c, stream);
   fprintf_filtered (stream, " = ");
-  fput_command_name_styled (c->alias_target, stream);
-  fprintf_filtered (stream, " %s\n", c->default_args.c_str ());
+  fput_command_name_styled (*c.alias_target, stream);
+  fprintf_filtered (stream, " %s\n", c.default_args.c_str ());
 }
 
 /* Print the definition of the aliases of CMD that have default args.  */
 
 static void
-fput_aliases_definition_styled (struct cmd_list_element *cmd,
+fput_aliases_definition_styled (const cmd_list_element &cmd,
 				struct ui_file *stream)
 {
-  for (cmd_list_element &alias : cmd->aliases)
+  for (const cmd_list_element &alias : cmd.aliases)
     if (!alias.default_args.empty ())
-      fput_alias_definition_styled (&alias, stream);
+      fput_alias_definition_styled (alias, stream);
 }
 
 
@@ -1357,21 +1357,21 @@ fput_aliases_definition_styled (struct cmd_list_element *cmd,
 */
 
 static void
-fput_command_names_styled (struct cmd_list_element *c,
+fput_command_names_styled (const cmd_list_element &c,
 			   bool always_fput_c_name, const char *postfix,
 			   struct ui_file *stream)
 {
-  if (always_fput_c_name || !c->aliases.empty ())
+  if (always_fput_c_name || !c.aliases.empty ())
     fput_command_name_styled (c, stream);
 
-  for (cmd_list_element &alias : c->aliases)
+  for (const cmd_list_element &alias : c.aliases)
     {
       fputs_filtered (", ", stream);
       wrap_here ("   ");
-      fput_command_name_styled (&alias, stream);
+      fput_command_name_styled (alias, stream);
     }
 
-  if (always_fput_c_name || !c->aliases.empty ())
+  if (always_fput_c_name || !c.aliases.empty ())
     fputs_filtered (postfix, stream);
 }
 
@@ -1380,7 +1380,7 @@ fput_command_names_styled (struct cmd_list_element *c,
    otherwise print only one-line help for command C.  */
 
 static void
-print_doc_of_command (struct cmd_list_element *c, const char *prefix,
+print_doc_of_command (const cmd_list_element &c, const char *prefix,
 		      bool verbose, compiled_regex &highlight,
 		      struct ui_file *stream)
 {
@@ -1396,12 +1396,12 @@ print_doc_of_command (struct cmd_list_element *c, const char *prefix,
     {
       fputs_filtered ("\n", stream);
       fput_aliases_definition_styled (c, stream);
-      fputs_highlighted (c->doc, highlight, stream);
+      fputs_highlighted (c.doc, highlight, stream);
       fputs_filtered ("\n", stream);
     }
   else
     {
-      print_doc_line (stream, c->doc, false);
+      print_doc_line (stream, c.doc, false);
       fputs_filtered ("\n", stream);
       fput_aliases_definition_styled (c, stream);
     }
@@ -1441,7 +1441,7 @@ apropos_cmd (struct ui_file *stream,
 	  /* Try to match against the name.  */
 	  returnvalue = regex.search (c->name, name_len, 0, name_len, NULL);
 	  if (returnvalue >= 0)
-	    print_doc_of_command (c, prefix, verbose, regex, stream);
+	    print_doc_of_command (*c, prefix, verbose, regex, stream);
 
 	  /* Try to match against the name of the aliases.  */
 	  for (const cmd_list_element &alias : c->aliases)
@@ -1450,7 +1450,7 @@ apropos_cmd (struct ui_file *stream,
 	      returnvalue = regex.search (alias.name, name_len, 0, name_len, NULL);
 	      if (returnvalue >= 0)
 		{
-		  print_doc_of_command (c, prefix, verbose, regex, stream);
+		  print_doc_of_command (*c, prefix, verbose, regex, stream);
 		  break;
 		}
 	    }
@@ -1461,7 +1461,7 @@ apropos_cmd (struct ui_file *stream,
 
 	  /* Try to match against documentation.  */
 	  if (regex.search (c->doc, doc_len, 0, doc_len, NULL) >= 0)
-	    print_doc_of_command (c, prefix, verbose, regex, stream);
+	    print_doc_of_command (*c, prefix, verbose, regex, stream);
 	}
       /* Check if this command has subcommands.  */
       if (c->is_prefix ())
@@ -1524,8 +1524,8 @@ help_cmd (const char *command, struct ui_file *stream)
 
   /* If the user asked 'help somecommand' and there is no alias,
      the false indicates to not output the (single) command name.  */
-  fput_command_names_styled (c, false, "\n", stream);
-  fput_aliases_definition_styled (c, stream);
+  fput_command_names_styled (*c, false, "\n", stream);
+  fput_aliases_definition_styled (*c, stream);
   fputs_filtered (c->doc, stream);
   fputs_filtered ("\n", stream);
 
@@ -1664,7 +1664,7 @@ help_all (struct ui_file *stream)
 	      fprintf_filtered (stream, "\nUnclassified commands\n\n");
 	      seen_unclassified = 1;
 	    }
-	  print_help_for_command (c, true, stream);
+	  print_help_for_command (*c, true, stream);
 	}
     }
 
@@ -1716,23 +1716,23 @@ print_doc_line (struct ui_file *stream, const char *str,
    If RECURSE is non-zero, also print one-line descriptions
    of all prefixed subcommands.  */
 static void
-print_help_for_command (struct cmd_list_element *c,
+print_help_for_command (const cmd_list_element &c,
 			bool recurse, struct ui_file *stream)
 {
   fput_command_names_styled (c, true, " -- ", stream);
-  print_doc_line (stream, c->doc, false);
+  print_doc_line (stream, c.doc, false);
   fputs_filtered ("\n", stream);
-  if (!c->default_args.empty ())
+  if (!c.default_args.empty ())
     fput_alias_definition_styled (c, stream);
   fput_aliases_definition_styled (c, stream);
 
   if (recurse
-      && c->is_prefix ()
-      && c->abbrev_flag == 0)
+      && c.is_prefix ()
+      && c.abbrev_flag == 0)
     /* Subcommands of a prefix command typically have 'all_commands'
        as class.  If we pass CLASS to recursive invocation,
        most often we won't see anything.  */
-    help_cmd_list (*c->subcommands, all_commands, true, stream);
+    help_cmd_list (*c.subcommands, all_commands, true, stream);
 }
 
 /*
@@ -1789,7 +1789,7 @@ help_cmd_list (struct cmd_list_element *list, enum command_class theclass,
 	     as this would show the (possibly very long) not very useful
 	     list of sub-commands of the aliased command.  */
 	  print_help_for_command
-	    (c,
+	    (*c,
 	     recurse && (theclass != class_alias || !c->is_alias ()),
 	     stream);
 	  continue;
-- 
2.33.1

[PATCH 3/3] gdb: don't show deprecated aliases

[Simon Marchi]

I don't think it's very useful to show deprecated aliases to the
user.  It encourages the user to use them, when the goal is the
opposite.

For example, before:

    (gdb) help set index-cache enabled
    set index-cache enabled, set index-cache off, set index-cache on
      alias set index-cache off = set index-cache enabled off
      alias set index-cache on = set index-cache enabled on
    Enable the index cache.
    When on, enable the use of the index cache.

    (gdb) help set index-cache on
    Warning: 'set index-cache on', an alias for the command 'set index-cache enabled', is deprecated.
    Use 'set index-cache enabled on'.

    set index-cache enabled, set index-cache off, set index-cache on
      alias set index-cache off = set index-cache enabled off
      alias set index-cache on = set index-cache enabled on
    Enable the index cache.
    When on, enable the use of the index cache.

After:

    (gdb) help set index-cache enabled
    Enable the index cache.
    When on, enable the use of the index cache.
    (gdb) help set index-cache on
    Warning: 'set index-cache on', an alias for the command 'set index-cache enabled', is deprecated.
    Use 'set index-cache enabled on'.

    Enable the index cache.
    When on, enable the use of the index cache.

Change-Id: I989b618a5ad96ba975367e9d16db95523cd57a4c
---
 gdb/cli/cli-decode.c                | 29 ++++++++++++++++++++++++++---
 gdb/testsuite/gdb.base/commands.exp | 22 ++++++++++++++++++++++
 2 files changed, 48 insertions(+), 3 deletions(-)

diff --git a/gdb/cli/cli-decode.c b/gdb/cli/cli-decode.c
index b8be3f54921f..9b965ea99891 100644
--- a/gdb/cli/cli-decode.c
+++ b/gdb/cli/cli-decode.c
@@ -1345,7 +1345,7 @@ fput_aliases_definition_styled (const cmd_list_element &cmd,
 				struct ui_file *stream)
 {
   for (const cmd_list_element &alias : cmd.aliases)
-    if (!alias.default_args.empty ())
+    if (!alias.cmd_deprecated && !alias.default_args.empty ())
       fput_alias_definition_styled (alias, stream);
 }
 
@@ -1361,17 +1361,40 @@ fput_command_names_styled (const cmd_list_element &c,
 			   bool always_fput_c_name, const char *postfix,
 			   struct ui_file *stream)
 {
-  if (always_fput_c_name || !c.aliases.empty ())
+  /* First, check if we are going to print something.  That is, either if
+     ALWAYS_FPUT_C_NAME is true or if there exists at least one non-deprecated
+     alias.  */
+
+  auto print_alias = [] (const cmd_list_element &alias)
+    {
+      return !alias.cmd_deprecated;
+    };
+
+  bool print_something = always_fput_c_name;
+  if (!print_something)
+    for (const cmd_list_element &alias : c.aliases)
+      {
+	if (!print_alias (alias))
+	  continue;
+
+	print_something = true;
+	break;
+      }
+
+  if (print_something)
     fput_command_name_styled (c, stream);
 
   for (const cmd_list_element &alias : c.aliases)
     {
+      if (!print_alias (alias))
+	continue;
+
       fputs_filtered (", ", stream);
       wrap_here ("   ");
       fput_command_name_styled (alias, stream);
     }
 
-  if (always_fput_c_name || !c.aliases.empty ())
+  if (print_something)
     fputs_filtered (postfix, stream);
 }
 
diff --git a/gdb/testsuite/gdb.base/commands.exp b/gdb/testsuite/gdb.base/commands.exp
index 6785f9532b11..1dca419f0fb7 100644
--- a/gdb/testsuite/gdb.base/commands.exp
+++ b/gdb/testsuite/gdb.base/commands.exp
@@ -709,6 +709,27 @@ maintenance deprecate set qqq_aaa"
     file delete $file1
 }
 
+# Test that the help for a command does not show deprecated aliases.
+
+proc_with_prefix deprecated_command_alias_help_test {} {
+    gdb_test_multiline "define real_command" \
+	"define real_command" "End with a line saying just \"end\".." \
+	"print 1" "" \
+	"end" ""
+
+    gdb_test_no_output "alias alias_command = real_command"
+    gdb_test_no_output "alias alias_with_args_command = real_command 123"
+
+    gdb_test "help real_command" \
+	"real_command, alias_with_args_command, alias_command\r\n  alias alias_with_args_command = real_command 123\r\nUser-defined." \
+	"help real_command, before"
+    gdb_test_no_output "maintenance deprecate alias_command"
+    gdb_test_no_output "maintenance deprecate alias_with_args_command"
+    gdb_test "help real_command" \
+	"User-defined." \
+	"help real_command, after"
+}
+
 proc_with_prefix bp_deleted_in_command_test {} {
     global gdb_prompt
 
@@ -1203,6 +1224,7 @@ user_defined_command_manyargs_test
 watchpoint_command_test
 test_command_prompt_position
 deprecated_command_test
+deprecated_command_alias_help_test
 bp_deleted_in_command_test
 temporary_breakpoint_commands
 stray_arg0_test
-- 
2.33.1

Re: [PATCH 1/3] gdb: use intrusive_list for cmd_list_element aliases list

[Pedro Alves]

On 2021-12-01 16:41, Simon Marchi via Gdb-patches wrote:
> Change the manually-implemented linked list to use intrusive_list.  This
> is not strictly necessary, but it makes the code much simpler.

LGTM.

Re: [PATCH 2/3] gdb: change some alias functions parameters to const-reference

[Pedro Alves]

On 2021-12-01 16:41, Simon Marchi via Gdb-patches wrote:
> Now that we use intrusive list to link aliases, it becomes easier to
> pass cmd_list_element arguments by const-reference rather than by
> pointer to some functions, change a few.
> 
> Change-Id: Id0df648ed26e9447da0671fc2c858981cda31df8

LGTM.

Re: [PATCH 3/3] gdb: don't show deprecated aliases

[Pedro Alves]

On 2021-12-01 16:41, Simon Marchi via Gdb-patches wrote:
> I don't think it's very useful to show deprecated aliases to the
> user.  It encourages the user to use them, when the goal is the
> opposite.
> 
> For example, before:
> 
>     (gdb) help set index-cache enabled
>     set index-cache enabled, set index-cache off, set index-cache on
>       alias set index-cache off = set index-cache enabled off
>       alias set index-cache on = set index-cache enabled on
>     Enable the index cache.
>     When on, enable the use of the index cache.
> 
>     (gdb) help set index-cache on
>     Warning: 'set index-cache on', an alias for the command 'set index-cache enabled', is deprecated.
>     Use 'set index-cache enabled on'.
> 
>     set index-cache enabled, set index-cache off, set index-cache on
>       alias set index-cache off = set index-cache enabled off
>       alias set index-cache on = set index-cache enabled on
>     Enable the index cache.
>     When on, enable the use of the index cache.
> 

I agree, this looks crowded, unclear and confusing.

> After:
> 
>     (gdb) help set index-cache enabled
>     Enable the index cache.
>     When on, enable the use of the index cache.
>     (gdb) help set index-cache on
>     Warning: 'set index-cache on', an alias for the command 'set index-cache enabled', is deprecated.
>     Use 'set index-cache enabled on'.
> 
>     Enable the index cache.
>     When on, enable the use of the index cache.
> 
> Change-Id: I989b618a5ad96ba975367e9d16db95523cd57a4c

LGTM.

> ---
>  gdb/cli/cli-decode.c                | 29 ++++++++++++++++++++++++++---
>  gdb/testsuite/gdb.base/commands.exp | 22 ++++++++++++++++++++++
>  2 files changed, 48 insertions(+), 3 deletions(-)
> 
> diff --git a/gdb/cli/cli-decode.c b/gdb/cli/cli-decode.c
> index b8be3f54921f..9b965ea99891 100644
> --- a/gdb/cli/cli-decode.c
> +++ b/gdb/cli/cli-decode.c
> @@ -1345,7 +1345,7 @@ fput_aliases_definition_styled (const cmd_list_element &cmd,
>  				struct ui_file *stream)
>  {
>    for (const cmd_list_element &alias : cmd.aliases)
> -    if (!alias.default_args.empty ())
> +    if (!alias.cmd_deprecated && !alias.default_args.empty ())
>        fput_alias_definition_styled (alias, stream);
>  }
>  
> @@ -1361,17 +1361,40 @@ fput_command_names_styled (const cmd_list_element &c,
>  			   bool always_fput_c_name, const char *postfix,
>  			   struct ui_file *stream)
>  {
> -  if (always_fput_c_name || !c.aliases.empty ())
> +  /* First, check if we are going to print something.  That is, either if
> +     ALWAYS_FPUT_C_NAME is true or if there exists at least one non-deprecated
> +     alias.  */
> +
> +  auto print_alias = [] (const cmd_list_element &alias)
> +    {
> +      return !alias.cmd_deprecated;
> +    };
> +
> +  bool print_something = always_fput_c_name;
> +  if (!print_something)
> +    for (const cmd_list_element &alias : c.aliases)
> +      {
> +	if (!print_alias (alias))
> +	  continue;
> +
> +	print_something = true;
> +	break;
> +      }
> +
> +  if (print_something)
>      fput_command_name_styled (c, stream);
>  
>    for (const cmd_list_element &alias : c.aliases)
>      {
> +      if (!print_alias (alias))
> +	continue;
> +
>        fputs_filtered (", ", stream);
>        wrap_here ("   ");
>        fput_command_name_styled (alias, stream);
>      }
>  
> -  if (always_fput_c_name || !c.aliases.empty ())
> +  if (print_something)
>      fputs_filtered (postfix, stream);
>  }
>  
> diff --git a/gdb/testsuite/gdb.base/commands.exp b/gdb/testsuite/gdb.base/commands.exp
> index 6785f9532b11..1dca419f0fb7 100644
> --- a/gdb/testsuite/gdb.base/commands.exp
> +++ b/gdb/testsuite/gdb.base/commands.exp
> @@ -709,6 +709,27 @@ maintenance deprecate set qqq_aaa"
>      file delete $file1
>  }
>  
> +# Test that the help for a command does not show deprecated aliases.
> +
> +proc_with_prefix deprecated_command_alias_help_test {} {
> +    gdb_test_multiline "define real_command" \
> +	"define real_command" "End with a line saying just \"end\".." \
> +	"print 1" "" \
> +	"end" ""
> +
> +    gdb_test_no_output "alias alias_command = real_command"
> +    gdb_test_no_output "alias alias_with_args_command = real_command 123"
> +
> +    gdb_test "help real_command" \
> +	"real_command, alias_with_args_command, alias_command\r\n  alias alias_with_args_command = real_command 123\r\nUser-defined." \
> +	"help real_command, before"
> +    gdb_test_no_output "maintenance deprecate alias_command"
> +    gdb_test_no_output "maintenance deprecate alias_with_args_command"
> +    gdb_test "help real_command" \
> +	"User-defined." \
> +	"help real_command, after"
> +}
> +
>  proc_with_prefix bp_deleted_in_command_test {} {
>      global gdb_prompt
>  
> @@ -1203,6 +1224,7 @@ user_defined_command_manyargs_test
>  watchpoint_command_test
>  test_command_prompt_position
>  deprecated_command_test
> +deprecated_command_alias_help_test
>  bp_deleted_in_command_test
>  temporary_breakpoint_commands
>  stray_arg0_test
> 

Re: [PATCH 3/3] gdb: don't show deprecated aliases

[Simon Marchi]

On 2021-12-03 11:16, Pedro Alves wrote:
> On 2021-12-01 16:41, Simon Marchi via Gdb-patches wrote:
>> I don't think it's very useful to show deprecated aliases to the
>> user.  It encourages the user to use them, when the goal is the
>> opposite.
>>
>> For example, before:
>>
>>     (gdb) help set index-cache enabled
>>     set index-cache enabled, set index-cache off, set index-cache on
>>       alias set index-cache off = set index-cache enabled off
>>       alias set index-cache on = set index-cache enabled on
>>     Enable the index cache.
>>     When on, enable the use of the index cache.
>>
>>     (gdb) help set index-cache on
>>     Warning: 'set index-cache on', an alias for the command 'set index-cache enabled', is deprecated.
>>     Use 'set index-cache enabled on'.
>>
>>     set index-cache enabled, set index-cache off, set index-cache on
>>       alias set index-cache off = set index-cache enabled off
>>       alias set index-cache on = set index-cache enabled on
>>     Enable the index cache.
>>     When on, enable the use of the index cache.
>>
> 
> I agree, this looks crowded, unclear and confusing.
> 
>> After:
>>
>>     (gdb) help set index-cache enabled
>>     Enable the index cache.
>>     When on, enable the use of the index cache.
>>     (gdb) help set index-cache on
>>     Warning: 'set index-cache on', an alias for the command 'set index-cache enabled', is deprecated.
>>     Use 'set index-cache enabled on'.
>>
>>     Enable the index cache.
>>     When on, enable the use of the index cache.
>>
>> Change-Id: I989b618a5ad96ba975367e9d16db95523cd57a4c
> 
> LGTM.

Thanks.

Philippe, do you have an opinion on this change of behavior?

Simon

Re: [PATCH 2/3] gdb: change some alias functions parameters to const-reference

[Simon Marchi]

On 2021-12-03 11:13, Pedro Alves wrote:
> On 2021-12-01 16:41, Simon Marchi via Gdb-patches wrote:
>> Now that we use intrusive list to link aliases, it becomes easier to
>> pass cmd_list_element arguments by const-reference rather than by
>> pointer to some functions, change a few.
>>
>> Change-Id: Id0df648ed26e9447da0671fc2c858981cda31df8
> 
> LGTM.
> 

Thanks, I pushed the first two patches, since they don't introduce
a change in behavior, while I wait for feedback from Philippe on the
3rd one.

Simon

Re: [PATCH 3/3] gdb: don't show deprecated aliases

[Philippe Waroquiers]

On Fri, 2021-12-03 at 16:48 -0500, Simon Marchi wrote:
> 
> On 2021-12-03 11:16, Pedro Alves wrote:
> > On 2021-12-01 16:41, Simon Marchi via Gdb-patches wrote:
> > 
> > 
> > > After:
> > > 
> > >     (gdb) help set index-cache enabled
> > >     Enable the index cache.
> > >     When on, enable the use of the index cache.
> > >     (gdb) help set index-cache on
> > >     Warning: 'set index-cache on', an alias for the command 'set index-cache enabled', is deprecated.
> > >     Use 'set index-cache enabled on'.
> > > 
> > >     Enable the index cache.
> > >     When on, enable the use of the index cache.
> > > 
> > > Change-Id: I989b618a5ad96ba975367e9d16db95523cd57a4c
> > 
> > LGTM.
> 
> Thanks.
> 
> Philippe, do you have an opinion on this change of behavior?
> 
> Simon
I agree that showing deprecated aliases is not useful,
and the 'After' output is much better.

Thanks
Philippe

Re: [PATCH 3/3] gdb: don't show deprecated aliases

[Simon Marchi]

On 2021-12-04 07:33, Philippe Waroquiers wrote:
> On Fri, 2021-12-03 at 16:48 -0500, Simon Marchi wrote:
>>
>> On 2021-12-03 11:16, Pedro Alves wrote:
>>> On 2021-12-01 16:41, Simon Marchi via Gdb-patches wrote:
>>>
>>>
>>>> After:
>>>>
>>>>     (gdb) help set index-cache enabled
>>>>     Enable the index cache.
>>>>     When on, enable the use of the index cache.
>>>>     (gdb) help set index-cache on
>>>>     Warning: 'set index-cache on', an alias for the command 'set index-cache enabled', is deprecated.
>>>>     Use 'set index-cache enabled on'.
>>>>
>>>>     Enable the index cache.
>>>>     When on, enable the use of the index cache.
>>>>
>>>> Change-Id: I989b618a5ad96ba975367e9d16db95523cd57a4c
>>>
>>> LGTM.
>>
>> Thanks.
>>
>> Philippe, do you have an opinion on this change of behavior?
>>
>> Simon
> I agree that showing deprecated aliases is not useful,
> and the 'After' output is much better.
> 
> Thanks
> Philippe
> 
> 

Ok, thanks, pushed.

Simon