Re: [RFC PATCH] gdb: add command to clear value history

[Bruno Larsen <blarsen@redhat.com>]

On 19/10/2022 05:39, Rustam Kovhaev via Gdb-patches wrote:
> Hello,

Hi Rustam!

Thanks for this patch! I am indifferent to the new command, as I never 
needed it but I can't see it impacting anything else, so I'm fine with 
it being added.

However, the command "clear" already exists. It clears all breakpoints. 
You could either add "values" as a subcommand to the existing command, 
by having it added around gdb/breakpoint.c:14434, but doing so might 
change how the bare "clear" command works (I'm not sure here), or add 
something like "values clear" instead, which I think it's a bit too big. 
This specific point I'll defer to other maintainers on helping you.

I also noticed a few specific things that I'll explain inline.

> While debugging an application I wanted to clear the value history and I
> could not find an option to do it.
> I might be the only one who gets lost in the value history and I am not
> sure if anyone else needs it, but I thought it would be nice to have it.
> And I hope this does not break anything.
> Please let me know what you think. Thanks!
>
> Let's have the functionality to clear the value history.
> Introduce 'set values clear' command to clear the value history.

GDB tends to prefer a more objective commit message style, even if it 
comes from a personal need. Your message is fine for the RFC but it 
needs to be iterated for it to go in. See commits 
e4014689b9a1b9aa0dde8f8a358401774566fe8b and 
70175292616118bad315296ba6180f375326bb6c for some exaples.

>
> Signed-off-by: Rustam Kovhaev <rkovhaev@gmail.com>
We don't really use Signed-off-by tags in GDB.
> ---
>   gdb/value.c | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
>
> diff --git a/gdb/value.c b/gdb/value.c
> index 605e52dee82..726b5ef9084 100644
> --- a/gdb/value.c
> +++ b/gdb/value.c
> @@ -1971,6 +1971,15 @@ show_values (const char *num_exp, int from_tty)
>     if (from_tty && num_exp)
>       set_repeat_arguments ("+");
>   }
> +
> +static struct cmd_list_element *setvallist;
> +
> +static void
> +clear_values (const char *ignore, int from_tty)
> +{
> +    value_history.clear ();
> +}
> +
>   \f
>   enum internalvar_kind
>   {
> @@ -4343,6 +4352,13 @@ Convenience functions are defined via the Python API."
>   Elements of value history around item number IDX (or last ten)."),
>   	   &showlist);
>   
> +  add_cmd("clear", no_class, clear_values, _("\
> +Clear value history."), &setvallist);
You forgot to add a space before ( here.
> +
> +  add_basic_prefix_cmd ("values", class_support, _("\
> +Generic command for setting value history parameters"),
> +		  &setvallist, 0, &setlist);
> +

Since you are using &setvallist and &setlist here, your command ends up 
being "set values clear" instead of "clear values". You probably want to 
use &cmdlist instead.

>     add_com ("init-if-undefined", class_vars, init_if_undefined_command, _("\
>   Initialize a convenience variable if necessary.\n\
>   init-if-undefined VARIABLE = EXPRESSION\n\

It would be good if you added a test for this command. I think it would 
go well in gdb.base/printcmd.exp, adding some extra lines like

gdb_test_no_output "clear values" "test clearing values"
gdb_test "print 1" ".1 = 1" "test that values were cleared"


With all this being said, I like the direction of your patch, I hope to 
see it going upstream soon :-)

Cheers,
Bruno