Re: [PATCH] TUI disassembly window improvement - patch inline

[Tom Tromey <tom@tromey.com>]

>>>>> "Vasili" == Vasili Burdo via Gdb-patches <gdb-patches@sourceware.org> writes:

Vasili> I implemented a small update to TUI disassembly window - see attachment.
Vasili> This patch removes function name from disassembly listing leaving only
Vasili> offset from function start.
Vasili> The  reason for it - disassembly TUI is almost unusable in case of C++
Vasili> functions - their names (especially templated ones) are very long and
Vasili> thus litter disassembly view.

Hi.  Thank you for your patch.

I like the idea, I think it's a nice improvement.

Currently, patches require a ChangeLog entry.
See https://sourceware.org/gdb/wiki/ContributionChecklist

Also, if you do not have a copyright assignment, then you will probably
need one.  You can email me off-list to get started.  We can't check in
a non-trivial patch without one.

Vasili> Please, look at sample screenshots here: https://imgur.com/a/GlkVXGi

I appreciate this, but I think it would be better if the commit message
didn't reference a URL like this.  Perhaps a description would be
better.

This is "PR tui/25347" (see
https://sourceware.org/bugzilla/show_bug.cgi?id=25347) so the commit
message ought to include that text.  This will ensure the commit is
mentioned in the bug.

Vasili> @@ -110,7 +113,7 @@ tui_disassemble (struct gdbarch *gdbarch,
Vasili>    asm_lines.clear ();

Vasili>    /* Now construct each line.  */
Vasili> -  for (int i = 0; i < count; ++i)
Vasili> +  while (asm_lines.size() < count)

gdb style is a space before an open paren.

Vasili> +      if(for_ui)

I didn't really understand why this function needed two modes.
I think it would be good to update the introductory comment before
tui_disassemble to explain the meaning of the new parameter.

Vasili> +          if (0 == build_address_symbolic (gdbarch, orig_pc,
Vasili> asm_demangle, false,

Your mailer seems to have mangled the patch.
This will make it harder to eventually apply it.

Vasili> +  /* Set title to current function name */
Vasili> +  title.clear();
Vasili> +  if (asm_lines.size())
Vasili> +    title = asm_lines.front().func_string;

Probably you can check '!asm_lines.empty ()' instead.

I wonder if the title setting is correct here.  What if the window
contents end up like:

    fn1:
      asm
      asm
      asm
    fn2:
      asm
      PC => asm
      asm

In this scenario, would the title be "fn1" or "fn2"? 

Also, which is better?  On the one hand, if the title just shows the
function at the top of the window, then scrolling down will work nicely
in a way -- when a function name scrolls off the top, it will stick in
the title.  On the other hand, I might expect the title to display the
function holding the PC.  I'm really not sure, your input is
appreciated.

I didn't try the patch, but it would be good to report how you tested
it.  I don't recall offhand if there are TUI tests that would need to be
updated.  When modifying the TUI like this, it's usually fine to just
re-run the gdb.tui tests.  If you're unfamiliar with dejagnu, just ask
and I can give you a recipe.

thanks again,
Tom