[v3] Align natural-format register values to the same column

  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. This patch changes the alignment to spaces only. The tests checking
the output and expecting the single tab have been fixed in a previous patch, so
this change doesn't break any.

gdb/ChangeLog:

	* infcmd.c (default_print_one_register_info): Align natural-format
	column values consistently one under another.
---
 gdb/infcmd.c |   37 ++++++++++++++++++++++++++++---------
 1 file changed, 28 insertions(+), 9 deletions(-)
  

Hi Ruslan,

The resulting output looks great.  I have a few comments, some of them
are identical to the review of v2.

On 2018-01-19 02:22 AM, 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. This patch changes the alignment to spaces only. The tests checking
> the output and expecting the single tab have been fixed in a previous patch, so
> this change doesn't break any.
>
> gdb/ChangeLog:
> 
> 	* infcmd.c (default_print_one_register_info): Align natural-format
> 	column values consistently one under another.

Please mention that pad_to_column was added.

> ---
>  gdb/infcmd.c |   37 ++++++++++++++++++++++++++++---------
>  1 file changed, 28 insertions(+), 9 deletions(-)
> 
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index 976276b..c59a8f8 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -2283,6 +2283,15 @@ path_command (const char *dirname, int from_tty)
>  }
>  
>  
> +static void
> +pad_to_column (string_file& stream, int col)

Put the & after the space.

> +{
> +  stream.putc (' '); /* at least one space must be printed to separate columns */

Use a capital letter and period at the end (with two spaces after).  Wrap at 80 columns.

> +  const int size = stream.size ();
> +  if (size < col)
> +      stream.puts (n_spaces (col - size));
> +}
> +
>  /* Print out the register NAME with value VAL, to FILE, in the default
>     fashion.  */
>  
> @@ -2293,9 +2302,17 @@ 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,
> +	/* Give enough room for "0x", 16 hex digits and two spaces in
> +	   preceding column. */
> +	value_column_2 = value_column_1+2+16+2,

Reduce the indentation of these lines to 6 spaces, and add spaces around plus signs.

> +    };
>  
> -  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)));

Use pad_to_column?

>  
>    print_raw_format = (value_entirely_available (val)
>  		      && !value_optimized_out (val));
> @@ -2314,14 +2331,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 (format_stream, value_column_2);
> +	  format_stream.puts ("(raw ");
> +	  print_hex_chars (&format_stream, valaddr, TYPE_LENGTH (regtype), byte_order,

This line is now too long.

>  			   true);
> -	  fprintf_filtered (file, ")");
> +	  format_stream.puts (")");

You can use putc, since it's a single character.

Thanks,

Simon
  

Hi Simon,

On 3 February 2018 at 07:46, Simon Marchi <simark@simark.ca> wrote:
> Hi Ruslan,
>
> The resulting output looks great.  I have a few comments, some of them
> are identical to the review of v2.

I'm extremely sorry for making you repeat your comments. I keep
missing some formatting bits. Are there any known-working options for
any linter/indenter so that I could automatically check/prettify my
GDB patches before posting? I've tried astyle, but it somehow messes
up spaces/tabs and doesn't seem to support half-indenting enum braces
(or I didn't use the correct options). GNU indent appears to confuse
C++ references with bitwise OR, putting spaces on both sides, and also
doesn't want to half-indent enums.

I've sent a new version of the patch.

>
> On 2018-01-19 02:22 AM, 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. This patch changes the alignment to spaces only. The tests checking
>> the output and expecting the single tab have been fixed in a previous patch, so
>> this change doesn't break any.
>>
>> gdb/ChangeLog:
>>
>>       * infcmd.c (default_print_one_register_info): Align natural-format
>>       column values consistently one under another.
>
> Please mention that pad_to_column was added.
>
>> ---
>>  gdb/infcmd.c |   37 ++++++++++++++++++++++++++++---------
>>  1 file changed, 28 insertions(+), 9 deletions(-)
>>
>> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
>> index 976276b..c59a8f8 100644
>> --- a/gdb/infcmd.c
>> +++ b/gdb/infcmd.c
>> @@ -2283,6 +2283,15 @@ path_command (const char *dirname, int from_tty)
>>  }
>>
>>
>> +static void
>> +pad_to_column (string_file& stream, int col)
>
> Put the & after the space.
>
>> +{
>> +  stream.putc (' '); /* at least one space must be printed to separate columns */
>
> Use a capital letter and period at the end (with two spaces after).  Wrap at 80 columns.
>
>> +  const int size = stream.size ();
>> +  if (size < col)
>> +      stream.puts (n_spaces (col - size));
>> +}
>> +
>>  /* Print out the register NAME with value VAL, to FILE, in the default
>>     fashion.  */
>>
>> @@ -2293,9 +2302,17 @@ 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,
>> +     /* Give enough room for "0x", 16 hex digits and two spaces in
>> +        preceding column. */
>> +     value_column_2 = value_column_1+2+16+2,
>
> Reduce the indentation of these lines to 6 spaces, and add spaces around plus signs.
>
>> +    };
>>
>> -  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)));
>
> Use pad_to_column?
>
>>
>>    print_raw_format = (value_entirely_available (val)
>>                     && !value_optimized_out (val));
>> @@ -2314,14 +2331,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 (format_stream, value_column_2);
>> +       format_stream.puts ("(raw ");
>> +       print_hex_chars (&format_stream, valaddr, TYPE_LENGTH (regtype), byte_order,
>
> This line is now too long.
>
>>                          true);
>> -       fprintf_filtered (file, ")");
>> +       format_stream.puts (")");
>
> You can use putc, since it's a single character.
>
> Thanks,
>
> Simon
>

Regards,
Ruslan
  

On 2018-02-03 06:43 AM, Ruslan Kabatsayev wrote:
> I'm extremely sorry for making you repeat your comments. I keep
> missing some formatting bits. Are there any known-working options for
> any linter/indenter so that I could automatically check/prettify my
> GDB patches before posting? I've tried astyle, but it somehow messes
> up spaces/tabs and doesn't seem to support half-indenting enum braces
> (or I didn't use the correct options). GNU indent appears to confuse
> C++ references with bitwise OR, putting spaces on both sides, and also
> doesn't want to half-indent enums.

I don't know of any tool/config that does it right out of the box, but if
you manage to find a solution that works nicely, please share it.  The problem
is often that if you run files through an automatic formatter, you'll have a huge
diff of little changes (fix existing style issues, change how things are wrapped,
etc).  If there was a way to tell the formatter to only touch the lines that were
touched in your patch, it would be ideal.

What do you mean by half-indent enum, do you mean that the curly braces are
indented with respect to the enum keywork, like this?

  enum tab_stops
    {
      value_column_1 = 15,
      /* Give enough room for "0x", 16 hex digits and two spaces in
         preceding column. */
      value_column_2 = value_column_1 + 2 + 16 + 2,
    };

I've seen inconsistent style in GDB between the above, and this:

  enum tab_stops
  {
    value_column_1 = 15,
    /* Give enough room for "0x", 16 hex digits and two spaces in
       preceding column. */
    value_column_2 = value_column_1 + 2 + 16 + 2,
  };

I think the second version makes more sense, because it mimics how it's
done for functions.  But I don't know if it should be different if the
enum is declared inside the scope of a function.  If your formatter
writes it like the second version, I would be totally fine with it.

> I've sent a new version of the patch.

Thanks, I'll look at it later.

Simon
  

On 3 February 2018 at 18:55, Simon Marchi <simark@simark.ca> wrote:
> On 2018-02-03 06:43 AM, Ruslan Kabatsayev wrote:
>> I'm extremely sorry for making you repeat your comments. I keep
>> missing some formatting bits. Are there any known-working options for
>> any linter/indenter so that I could automatically check/prettify my
>> GDB patches before posting? I've tried astyle, but it somehow messes
>> up spaces/tabs and doesn't seem to support half-indenting enum braces
>> (or I didn't use the correct options). GNU indent appears to confuse
>> C++ references with bitwise OR, putting spaces on both sides, and also
>> doesn't want to half-indent enums.
>
> I don't know of any tool/config that does it right out of the box, but if
> you manage to find a solution that works nicely, please share it.  The problem
> is often that if you run files through an automatic formatter, you'll have a huge
> diff of little changes (fix existing style issues, change how things are wrapped,
> etc).  If there was a way to tell the formatter to only touch the lines that were
> touched in your patch, it would be ideal.
>
> What do you mean by half-indent enum, do you mean that the curly braces are
> indented with respect to the enum keywork, like this?
>
>   enum tab_stops
>     {
>       value_column_1 = 15,
>       /* Give enough room for "0x", 16 hex digits and two spaces in
>          preceding column. */
>       value_column_2 = value_column_1 + 2 + 16 + 2,
>     };

I guess I was still under the impression of 4-space indent between
scopes and perceived this additional 2-space indent of braces after
enum as a half-indent. I see now that generally the indent is 2
spaces, not 4, so this is actually one full indentation of the braces.

>
> I've seen inconsistent style in GDB between the above, and this:
>
>   enum tab_stops
>   {
>     value_column_1 = 15,
>     /* Give enough room for "0x", 16 hex digits and two spaces in
>        preceding column. */
>     value_column_2 = value_column_1 + 2 + 16 + 2,
>   };

Apparently I wasn't too lucky when I was looking for examples how to
format enums in the code base and came up with the first instance of
"enum opt" in "gdb/mi/mi-cmd-stack.c". Otherwise I'd do it without
indenting braces.

>
> I think the second version makes more sense, because it mimics how it's
> done for functions.  But I don't know if it should be different if the
> enum is declared inside the scope of a function.  If your formatter
> writes it like the second version, I would be totally fine with it.
>
>> I've sent a new version of the patch.
>
> Thanks, I'll look at it later.
>
> Simon