Re: [PATCH 15/24] Introduce rename_cmd

[Pedro Alves <palves@redhat.com>]

On 5/25/19 8:58 PM, Philippe Waroquiers wrote:
> On Wed, 2019-05-22 at 21:53 +0100, Pedro Alves wrote:
> 
>> diff --git a/gdb/command.h b/gdb/command.h
>> index 35006cc339e..ed848114b85 100644
>> --- a/gdb/command.h
>> +++ b/gdb/command.h
>> @@ -439,6 +439,11 @@ extern void
>>  				       struct cmd_list_element **set_list,
>>  				       struct cmd_list_element **show_list);
>>  
>> +/* Move command OLD_NAME from OLD_LIST to NEW_LIST and rename it from
>> +   OLD_NAME to NEW_NAME.  */
>> +extern void rename_cmd (const char *old_name, cmd_list_element **old_list,
>> +			const char *new_name, cmd_list_element **new_list);
>> +
> I have some difficulties to understand the idea here.
> If the command is moved from one list to another, how is the renamed
> command still available via the 'normal' list ?

So the existing command is called 

 "set print raw frame-arguments"

I think "set print raw-frame-arguments" would have been
a better command name, but since that command already exists with
that spelling since gdb 7.7, and I didn't have a huge reason to
change it, in the end I opted to leave it as it was.

With that decision, the next problem is, how to implement both the
option and the set command without duplicating the help bits.
So what I did was, define the option like this:

 +  boolean_option_def {
 +    "raw-frame-arguments",
 +    [] (frame_print_options *opt) { return &opt->print_raw_frame_arguments; },
 +    NULL, /* show_cmd_cb */
 +    N_("Set whether to print frame arguments in raw form."),
 +    N_("Show whether to print frame arguments in raw form."),
 +    N_("If set, frame arguments are printed in raw form, bypassing any\n\
 +pretty-printers for that value.")
 +  },

and then let:

 +  gdb::option::add_setshow_cmds_for_options
 +    (class_stack, &user_frame_print_options,
 +     frame_print_option_defs, &setprintlist, &showprintlist);
 +

create the "set print raw-frame-arguments" command from the option definition
shown above.

And now that's where the renaming takes place, with:

+  /* The above installs a "set print raw-frame-arguments" command,
+     because there's an option called "print -raw-frame-arguments".
+     Rename the command to "set print raw frame-arguments" (space
+     instead of dash), to keep backward compatibility -- the "raw
+     frame-arguments" command already existed when print options were
+     first added.  */
+  rename_cmd ("raw-frame-arguments", &setprintlist,
+             "frame-arguments", &setprintrawlist);
+  rename_cmd ("raw-frame-arguments", &showprintlist,
+             "frame-arguments", &showprintrawlist);


There are other ways to implement this.  I could move the
"raw-frame-arguments" option_def out of the frame_print_option_defs
array, so that it doesn't get installed as a command, and leave the
"set print raw frame-arguments" add_setshow... call in place.
I guess it wouldn't be a big deal.

> 
> Effectively, it looks like set/show print raw-frame-arguments commands
> are not known.

Yes, that was the intention.

> 
> Why not just keep the old command untouched, and define a new
> command via the new framework.
> 

It didn't seem worth it to me.

> Below is just a patch (based on HEAD) that defines 2 different set/show
> changing the same variable:
> (gdb) apropos raw form
> set print raw frame-arguments -- Set whether to print frame arguments in raw form
> set print raw-frame-arguments -- Set whether to print frame arguments in raw form
> show print raw frame-arguments -- Show whether to print frame arguments in raw form
> show print raw-frame-arguments -- Show whether to print frame arguments in raw form
> (gdb) set print raw frame-arguments on
> (gdb) show print raw frame-arguments 
> Whether to print frame arguments in raw form is on.
> (gdb) show print raw-frame-arguments 
> Whether to print frame arguments in raw form is on.
> (gdb) set print raw-frame-arguments off
> (gdb) show print raw frame-arguments
> Whether to print frame arguments in raw form is off.
> (gdb) show print raw-frame-arguments 
> Whether to print frame arguments in raw form is off.
> (gdb) 
> 
> I guess this should still work if the same variable is changed via
> the new framework and/or via add_setshow_boolean_cmd.

TBC, the new framework does not change how the set
commands work at all.  The integration that exists is that
the new gdb::option::add_setshow_cmds_for_options function
goes over the option definitions and calls add_setshow_xxx_cmd
for each option.  The option_def structure has some fields that
only exists for this, such as the show_doc, and the show_cmd_cb.
That was all done as a way to conveniently describe the help
bits for both the options and the commands in a single place.

> The old commands might be marked as obsolete.

Yes, if we add a "raw-frame-arguments", we should deprecate
"raw frame-arguments" so that TAB completion doesn't
suggest the latter, which would get in the way
of "set print raw[TAB]".

I actually tried doing that, and I don't recall the details,
but it didn't seem that easy, because there's more
than one command involved -- "raw" is a command, and then
"frame-arguments" is a subcommand.  I think it was at that
point that I had decided to just leave it as it was.

With the info above, any preference on how to proceed?

Thanks,
Pedro Alves