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

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

[Rustam Kovhaev]

Hello,

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.

Signed-off-by: Rustam Kovhaev <rkovhaev@gmail.com>
---
 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);
+
+  add_basic_prefix_cmd ("values", class_support, _("\
+Generic command for setting value history parameters"),
+		  &setvallist, 0, &setlist);
+
   add_com ("init-if-undefined", class_vars, init_if_undefined_command, _("\
 Initialize a convenience variable if necessary.\n\
 init-if-undefined VARIABLE = EXPRESSION\n\
-- 
2.37.3

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

[Bruno Larsen]

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

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

[Tom Tromey]

>>>>> "Rustam" == Rustam Kovhaev via Gdb-patches <gdb-patches@sourceware.org> writes:

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

Thanks for the patch.

In addition to what Bruno said, a new feature should have a
documentation patch and also a note in NEWS.

Rustam> +static void
Rustam> +clear_values (const char *ignore, int from_tty)
Rustam> +{
Rustam> +    value_history.clear ();

This is indented too much.

Tom

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

[Rustam Kovhaev]

On Thu, Oct 20, 2022 at 12:44:38PM -0600, Tom Tromey wrote:
> 
> Thanks for the patch.
> 
> In addition to what Bruno said, a new feature should have a
> documentation patch and also a note in NEWS.
> 
Hi Bruno, Tom,
Thanks for the review and the comments. I will send a proper patch.