Re: [PATCH v2] Align natural-format register values to the same column

[Ruslan Kabatsayev <b7 dot 10110111 at gmail dot com>]

Hi Simon,

On 26.07.2017 12:33, Simon Marchi wrote:
> Hi Ruslan,
> 
> Thanks for the update, I just have some minor comments, but otherwise 
> the patch looks good to me.
> 
> On 2017-07-26 09:53, Ruslan Kabatsayev wrote:
>> Currently, commands such as "info reg", "info all-reg", as well as 
>> register
>> window in the TUI print badly aligned columns, like here:
>>
>> eax            0x1      1
>> ecx            0xffffd3e0       -11296
>> edx            0xffffd404       -11260
>> ebx            0xf7fa5ff4       -134586380
>> esp            0xffffd390       0xffffd390
>> ebp            0xffffd3c8       0xffffd3c8
>> esi            0x0      0
>> edi            0x0      0
>> eip            0x8048b60        0x8048b60 <main+16>
>> eflags         0x286    [ PF SF IF ]
>> cs             0x23     35
>> ss             0x2b     43
>> ds             0x2b     43
>> es             0x2b     43
>> fs             0x0      0
>> gs             0x63     99
>>
>> After this patch, these commands print the third column values 
>> consistently
>> aligned one under another, provided the second column is not too long.
>> Originally, the third column was (attempted to be) aligned using a 
>> simple tab
>> character. Lots of tests in the test suite check for it, so this patch 
>> retains
>> the tab in the output after the second column. This allows these tests 
>> to
>> continue working unchanged. What is different is that now the tab may 
>> be
>> followed by several spaces, which complete the task of aligning the 
>> third
>> column when the sole tab doesn't work well.
>>
>> gdb/ChangeLog:
>>
>> 	* infcmd.c (default_print_one_register_info): Align natural-
>> 	format column value consistently one after another.
>> ---
>>  gdb/infcmd.c |   35 ++++++++++++++++++++++++++---------
>>  1 file changed, 26 insertions(+), 9 deletions(-)
>>
>> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
>> index defa7b0..56cdee2 100644
>> --- a/gdb/infcmd.c
>> +++ b/gdb/infcmd.c
>> @@ -2270,6 +2270,15 @@ path_command (char *dirname, int from_tty)
>>  }
>>  
>>
> 
> Please add a short comment to explain what this function does.
> 
>> +static void
>> +pad_to_column_prepending_tab (string_file& stream, int col)
> 
> The & should go after the space.
> 
>> +{
>> +  const int size_with_tab = stream.size () / 8 * 8 + 8;
>> +  stream.putc ('\t');
>> +  if (size_with_tab < col)
>> +      stream.puts (n_spaces (col - size_with_tab));
>> +}
>> +
>>  /* Print out the register NAME with value VAL, to FILE, in the default
>>     fashion.  */
>>
>> @@ -2280,9 +2289,15 @@ default_print_one_register_info (struct ui_file 
>> *file,
>>  {
>>    struct type *regtype = value_type (val);
>>    int print_raw_format;
>> +  string_file format_stream;
>> +  enum tab_stops
>> +    {
>> +	value_column_1=15,
>> +	value_column_2=32,
> 
> Perhaps those could have more explicit names, like "column_hex" and 
> "column_natural"?
This was my first thought, but then I noticed that for floating-point
registers we have "(raw 0x....)" on the right column and "1.32536e+52"
on the left one. Do you still think it's OK to name the left column hex
and the right one natural?
> 
> Formatting nit: indent those two lines with two columns less (so with 6 
> spaces in total) and add spaces around the =.
> 
>> +    };
>>
>> -  fputs_filtered (name, file);
>> -  print_spaces_filtered (15 - strlen (name), file);
>> +  format_stream.puts (name);
>> +  format_stream.puts (n_spaces (value_column_1 - strlen (name)));
>>
>>    print_raw_format = (value_entirely_available (val)
>>  		      && !value_optimized_out (val));
>> @@ -2301,14 +2316,15 @@ default_print_one_register_info (struct ui_file 
>> *file,
>>
>>        val_print (regtype,
>>  		 value_embedded_offset (val), 0,
>> -		 file, 0, val, &opts, current_language);
>> +		 &format_stream, 0, val, &opts, current_language);
>>
>>        if (print_raw_format)
>>  	{
>> -	  fprintf_filtered (file, "\t(raw ");
>> -	  print_hex_chars (file, valaddr, TYPE_LENGTH (regtype), byte_order,
>> +	  pad_to_column_prepending_tab (format_stream, value_column_2);
>> +	  format_stream.puts ("(raw ");
>> +	  print_hex_chars (&format_stream, valaddr, TYPE_LENGTH (regtype), 
>> byte_order,
>>  			   true);
>> -	  fprintf_filtered (file, ")");
>> +	  format_stream.puts (")");
>>  	}
>>      }
>>    else
>> @@ -2320,20 +2336,21 @@ default_print_one_register_info (struct ui_file 
>> *file,
>>        opts.deref_ref = 1;
>>        val_print (regtype,
>>  		 value_embedded_offset (val), 0,
>> -		 file, 0, val, &opts, current_language);
>> +		 &format_stream, 0, val, &opts, current_language);
>>        /* If not a vector register, print it also according to its
>>  	 natural format.  */
>>        if (print_raw_format && TYPE_VECTOR (regtype) == 0)
>>  	{
>> +	  pad_to_column_prepending_tab (format_stream, value_column_2);
>>  	  get_user_print_options (&opts);
>>  	  opts.deref_ref = 1;
>> -	  fprintf_filtered (file, "\t");
>>  	  val_print (regtype,
>>  		     value_embedded_offset (val), 0,
>> -		     file, 0, val, &opts, current_language);
>> +		     &format_stream, 0, val, &opts, current_language);
>>  	}
>>      }
>>
>> +  fputs_filtered (format_stream.c_str (), file);
>>    fprintf_filtered (file, "\n");
>>  }
> 
> Thanks,
> 
> Simon
> 

Regards,
Ruslan