| Message ID | 1516346545-8217-1-git-send-email-b7.10110111@gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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) +{ + stream.putc (' '); /* at least one space must be printed to separate 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, + }; - 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)); @@ -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, true); - fprintf_filtered (file, ")"); + format_stream.puts (")"); } } else @@ -2333,20 +2351,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 (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"); }