Re: [PATCH v2] Fix first time you type UP or DOWN in TUI's command window (Re: [RFC 8.3 0/3] Some style fixes)

Pedro Alves
2019-03-18T20:24:43Z

On 03/15/2019 03:37 PM, Eli Zaretskii wrote:
>> Cc: gdb-patches@sourceware.org
>> From: Pedro Alves <palves@redhat.com>
>> Date: Fri, 15 Mar 2019 14:15:23 +0000
>>
>> Bingo.
>>
>> So I think this is the right fix:
>>
>> >From 781ed3b6d82a4fb54f7bfe59185f0e6e9efd6b59 Mon Sep 17 00:00:00 2001
>> From: Pedro Alves <palves@redhat.com>
>> Date: Fri, 15 Mar 2019 13:05:26 +0000
>> Subject: [PATCH] Fix first time you type UP or DOWN in TUI's command window
>>
>> The first time you type UP or DOWN arrow in the command window, GDB
>> should scroll the source window, but instead it displays the line
>> number and the file name in the command window(?).
>>
>> What happens there is that the first time we call
>> tui_ui_out::do_field_int, it doesn't initialize m_line, because
>> m_start_of_line is -1, as set by the constructor; and then the
>> following call to tui_ui_out::do_field_string falls back to
>> cli_ui_out::do_field_string because m_line is zero.
>>
>> The problem is caused by a typo in the C++ification of tui_ui_out,
>> commit 112e8700a6f, where m_line and m_start_of_line's initial values
>> were swapped from what they used to be:
>
> Thanks, this sound right to me.

I've merged the patch to master and the 8.3 branch.

> Still, I'd welcome some comments in
> the header which explain the semantics of non-positive values of these
> members, and for m_start_of_line, also its role in general. Fixing
> this bug could have been much easier if that information was
> available to begin with.
For sure. This code predates me by a long shot. I wouldn't be
surprised if it was already in the original TUI dump from HP.

Anyway, I've stared at this for a while, and I _think_ this captures
the idea. WDYT?

From 8e37e7076f6ddca767db35b66284268935bcc186 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Mon, 18 Mar 2019 19:10:42 +0000
Subject: [PATCH] Add comments describing tui_ui_out and its fields, cleanup a
bit

This commit add comments describing tui_ui_out and its fields, and
cleans up the code a little bit.

Also switch to using in-class initialization so that the initial
values can be seen alongside the comments.

I see no reason for initializing m_line as -1 instead of 0, since all
the checks in the .c file are of the form "> 0". AFAICS there's no
practical difference between -1 and 0. So it seems simpler to
initialize it as 0.

There's a bit of redundancy in tui_ui_out::do_field_string, which is
fixed by this commit.

gdb/ChangeLog:
2019-03-18 Pedro Alves <palves@redhat.com>

* tui/tui-out.c (tui_ui_out::do_field_string): Simplify.
(tui_ui_out::do_text): Add comments. Reset M_LINE to 0 instead of
to -1. Fix TABs vs spaces.
(tui_ui_out::tui_ui_out): Don't initialize fields here.
* tui/tui-out.h (tui_ui_out) Add intro comments.
<m_line, m_start_of_line>: In-class initialize, and add describing
comment.
---
gdb/tui/tui-out.c | 27 +++++++++++++--------------
gdb/tui/tui-out.h | 21 +++++++++++++++++++--
2 files changed, 32 insertions(+), 16 deletions(-)

diff --git a/gdb/tui/tui-out.c b/gdb/tui/tui-out.c
index dd37736c4a..64f77077c8 100644
--- a/gdb/tui/tui-out.c
+++ b/gdb/tui/tui-out.c
@@ -57,17 +57,13 @@ tui_ui_out::do_field_string (int fldno, int width, ui_align align,
if (suppress_output ())
return;

+ m_start_of_line++;
+
if (fldname && m_line > 0 && strcmp (fldname, "fullname") == 0)
{
- m_start_of_line++;
- if (m_line > 0)
- {
- tui_show_source (string, m_line);
- }
+ tui_show_source (string, m_line);
return;
}
-
- m_start_of_line++;

cli_ui_out::do_field_string (fldno, width, align, fldname, string, style);
}
@@ -94,11 +90,16 @@ tui_ui_out::do_text (const char *string)
m_start_of_line++;
if (m_line > 0)
{
+ /* Printing a source line, so suppress regular output -- the
+ line was shown on the TUI's source window by tui_show_source
+ above instead. */
if (strchr (string, '\n') != 0)
- {
- m_line = -1;
- m_start_of_line = 0;
- }
+ {
+ /* If we've reached the end of the line, so go back to
+ letting text output go to the console. */
+ m_line = 0;
+ m_start_of_line = 0;
+ }
return;
}
if (strchr (string, '\n'))
@@ -108,9 +109,7 @@ tui_ui_out::do_text (const char *string)
}

tui_ui_out::tui_ui_out (ui_file *stream)
-: cli_ui_out (stream, 0),
- m_line (-1),
- m_start_of_line (0)
+ : cli_ui_out (stream, 0)
{
}

diff --git a/gdb/tui/tui-out.h b/gdb/tui/tui-out.h
index 10311c9255..b0d8b8d898 100644
--- a/gdb/tui/tui-out.h
+++ b/gdb/tui/tui-out.h
@@ -20,6 +20,10 @@

#include "cli-out.h"

+/* An ui_out class for the TUI. This is just like the CLI's ui_out,
+ except that it overrides output methods to detect when a source
+ line is being printed and show the source in the TUI's source
+ window instead of printing the line in the console window. */
class tui_ui_out : public cli_ui_out
{
public:
@@ -39,8 +43,21 @@ protected:

private:

- int m_line;
- int m_start_of_line;
+ /* These fields are used to make print_source_lines show the source
+ in the TUI's source window instead of in the console.
+ M_START_OF_LINE is incremented whenever something is output to
+ the ui_out. If an integer field named "line" is printed on the
+ ui_out, and nothing else has been printed yet (both
+ M_START_OF_LINE and M_LINE are still 0), we assume
+ print_source_lines is starting to print a source line, and thus
+ record the line number in M_LINE. Afterwards, when we see a
+ string field named "fullname" being output, we take the fullname
+ and the recorded line and show the source line in the TUI's
+ source window. tui_ui_out::do_text() suppresses text output
+ until it sees an endline being printed, at which point these
+ variables are reset back to 0. */
+ int m_line = 0;
+ int m_start_of_line = 0;
};

extern tui_ui_out *tui_out_new (struct ui_file *stream);
--
2.14.4