[PATCH] Style history variable output

[PATCH] Style history variable output

[Tom Tromey]

When printing a value, I think the history reference -- the "$1" in
the output -- should be styled using the "variable" style.  This patch
implements this.
---
 gdb/printcmd.c                   | 3 ++-
 gdb/testsuite/gdb.base/style.exp | 6 +++++-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index d8d97493bab..204dbfc8219 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -1253,7 +1253,8 @@ print_value (value *val, const value_print_options &opts)
 
   annotate_value_history_begin (histindex, val->type ());
 
-  gdb_printf ("$%d = ", histindex);
+  gdb_printf ("%p[$%d%p] = ", variable_name_style.style ().ptr (),
+	      histindex, nullptr);
 
   annotate_value_history_value ();
 
diff --git a/gdb/testsuite/gdb.base/style.exp b/gdb/testsuite/gdb.base/style.exp
index 0370550d251..0e64ed35c2b 100644
--- a/gdb/testsuite/gdb.base/style.exp
+++ b/gdb/testsuite/gdb.base/style.exp
@@ -196,8 +196,12 @@ proc run_style_tests { } {
 	set sfield [limited_style string_field variable]
 	set efield [limited_style e_field variable]
 	set evalue [limited_style VALUE_TWO variable]
+	# The two parts of the left hand side are styled separately.
+	# This is just an oddity of the current implementation.
+	set lhs \
+	    [limited_style "\\\$" variable][limited_style "$decimal" variable]
 	gdb_test "print struct_value" \
-	    "\{$ifield = 23,.*$sfield = .*,.*$efield = $evalue.*"
+	    "$lhs = \{$ifield = 23,.*$sfield = .*,.*$efield = $evalue.*"
 
 	set ffield [limited_style field variable]
 	set cstart [string_to_regexp "/* XXX "]
-- 
2.39.2

Re: [PATCH] Style history variable output

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

> When printing a value, I think the history reference -- the "$1" in
> the output -- should be styled using the "variable" style.  This patch
> implements this.

I'm still not 100% sold on the use of variable style, but I do agree
that styling the $%d part is a good idea, and short of creating a new
style (which feels like overkill), variable does seem like the best
choice.

> ---
>  gdb/printcmd.c                   | 3 ++-
>  gdb/testsuite/gdb.base/style.exp | 6 +++++-
>  2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/gdb/printcmd.c b/gdb/printcmd.c
> index d8d97493bab..204dbfc8219 100644
> --- a/gdb/printcmd.c
> +++ b/gdb/printcmd.c
> @@ -1253,7 +1253,8 @@ print_value (value *val, const value_print_options &opts)
>  
>    annotate_value_history_begin (histindex, val->type ());
>  
> -  gdb_printf ("$%d = ", histindex);
> +  gdb_printf ("%p[$%d%p] = ", variable_name_style.style ().ptr (),
> +	      histindex, nullptr);

How about:

  std::string idx = string_printf ("$%d", histindex);
  gdb_printf ("%ps = ", styled_string (variable_name_style.style (),
				       idx.c_str ()));

Then the '$' and the '%d' part will be styled as one...

>  
>    annotate_value_history_value ();
>  
> diff --git a/gdb/testsuite/gdb.base/style.exp b/gdb/testsuite/gdb.base/style.exp
> index 0370550d251..0e64ed35c2b 100644
> --- a/gdb/testsuite/gdb.base/style.exp
> +++ b/gdb/testsuite/gdb.base/style.exp
> @@ -196,8 +196,12 @@ proc run_style_tests { } {
>  	set sfield [limited_style string_field variable]
>  	set efield [limited_style e_field variable]
>  	set evalue [limited_style VALUE_TWO variable]
> +	# The two parts of the left hand side are styled separately.
> +	# This is just an oddity of the current implementation.
> +	set lhs \
> +	    [limited_style "\\\$" variable][limited_style "$decimal" variable]

... and this can be simplified?

Thanks,
Andrew

>  	gdb_test "print struct_value" \
> -	    "\{$ifield = 23,.*$sfield = .*,.*$efield = $evalue.*"
> +	    "$lhs = \{$ifield = 23,.*$sfield = .*,.*$efield = $evalue.*"
>  
>  	set ffield [limited_style field variable]
>  	set cstart [string_to_regexp "/* XXX "]
> -- 
> 2.39.2

Re: [PATCH] Style history variable output

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:

[ old patch ]

>> -  gdb_printf ("$%d = ", histindex);
>> +  gdb_printf ("%p[$%d%p] = ", variable_name_style.style ().ptr (),
>> +	      histindex, nullptr);

Andrew> How about:

Andrew>   std::string idx = string_printf ("$%d", histindex);
Andrew>   gdb_printf ("%ps = ", styled_string (variable_name_style.style (),
Andrew> 				       idx.c_str ()));

Andrew> Then the '$' and the '%d' part will be styled as one...

I spent some time trying to fix gdb_printf here, but eventually gave up
on it.  It's probably doable but a bit tricky.

Meanwhile I've applied your suggestion and I'm going to finally check
this in.

Tom