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

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

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