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

Message ID bc92ee1d-78d4-f3e8-94d8-c0473f357006@redhat.com
State New
Headers show
Series
  • [v2] Fix first time you type UP or DOWN in TUI's command window (Re: [RFC 8.3 0/3] Some style fixes)
Related show

Commit Message

Pedro Alves March 15, 2019, 2:15 p.m.
On 03/12/2019 04:44 PM, Eli Zaretskii wrote:
> 3. This patch fixes another problem noticed by Pedro: 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 fix below is

> perhaps somewhat ad-hoc, mainly because I couldn't understand the

> semantics of the values of m_start_of_line, especially its

> non-positive values; please consider documenting them in the header.

> Maybe if someone explains the semantics, I could come up with a more

> sound patch (or conclude that the below is TRT).  Also, it looks like

> the second test for m_line > 0 in tui_ui_out::do_field_string is

> redundant?

> 

> --- gdb/tui/tui-out.c~4	2019-02-27 06:51:50.000000000 +0200

> +++ gdb/tui/tui-out.c	2019-03-12 12:30:23.924230000 +0200

> @@ -34,6 +34,9 @@ tui_ui_out::do_field_int (int fldno, int

>    if (suppress_output ())

>      return;

>  

> +  if (m_start_of_line < 0 && m_line == 0)

> +    m_start_of_line = 0;

> +

>    /* Don't print line number, keep it for later.  */

>    if (m_start_of_line == 0 && strcmp (fldname, "line") == 0)

>      {

> 


I noticed that m_start_of_line never goes back to -1.
It is reset to 0 here:

void
tui_ui_out::do_text (const char *string)
{
...
      if (strchr (string, '\n') != 0)
        {
          m_line = -1;
          m_start_of_line = 0;
        }


and notice how m_line is reset to -1.  This is the exact
opposite of how the fields are initialized in the ctor:

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

... which made me suspect of a typo in the C++ification
of tui_ui_out.  Looking at that commit, 112e8700a6f, we see:

 -struct ui_out *
 -tui_out_new (struct ui_file *stream)
 +tui_ui_out::tui_ui_out (ui_file *stream)
 +: cli_ui_out (stream, 0),
 +  m_line (0),
 +  m_start_of_line (-1)
  {
 -
 -  /* Initialize our fields.  */
 -  data->line = -1;
 -  data->start_of_line = 0;

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:

 -struct ui_out *
 -tui_out_new (struct ui_file *stream)
 +tui_ui_out::tui_ui_out (ui_file *stream)
 +: cli_ui_out (stream, 0),
 +  m_line (0),
 +  m_start_of_line (-1)
  {
 -
 -  /* Initialize our fields.  */
 -  data->line = -1;
 -  data->start_of_line = 0;

This commit fixes it.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>
	    Eli Zaretskii <eliz@gnu.org>

	* tui/tui-out.c (tui_ui_out::tui_ui_out): Fix initialization of
	m_line and m_start_of_line.
---
 gdb/tui/tui-out.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

-- 
2.14.4

Comments

Eli Zaretskii March 15, 2019, 3:37 p.m. | #1
> 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.  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.
Pedro Alves March 18, 2019, 8:24 p.m. | #2
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
Eli Zaretskii March 19, 2019, 6:08 a.m. | #3
> Cc: tromey@adacore.com, gdb-patches@sourceware.org

> From: Pedro Alves <palves@redhat.com>

> Date: Mon, 18 Mar 2019 20:24:28 +0000

> 

> > 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?


Thanks, LGTM.  It certainly explains the values of these two members
and how they are used.

> +	{

> +	  /* If we've reached the end of the line, so go back to

> +	     letting text output go to the console.  */


A minor typo: "If ..., so ..." sounds weird.  I think you wanted to
get rid of that "If".
Pedro Alves March 19, 2019, 6:14 p.m. | #4
On 03/19/2019 06:08 AM, Eli Zaretskii wrote:
>> Cc: tromey@adacore.com, gdb-patches@sourceware.org

>> From: Pedro Alves <palves@redhat.com>


>> Anyway, I've stared at this for a while, and I _think_ this captures

>> the idea.  WDYT?

> 

> Thanks, LGTM.  It certainly explains the values of these two members

> and how they are used.

> 

>> +	{

>> +	  /* If we've reached the end of the line, so go back to

>> +	     letting text output go to the console.  */

> 

> A minor typo: "If ..., so ..." sounds weird.  I think you wanted to

> get rid of that "If".


Indeed.  Fixed that, and merged to master.

Thanks,
Pedro Alves

Patch

diff --git a/gdb/tui/tui-out.c b/gdb/tui/tui-out.c
index d5a173b94a..dd37736c4a 100644
--- a/gdb/tui/tui-out.c
+++ b/gdb/tui/tui-out.c
@@ -109,8 +109,8 @@  tui_ui_out::do_text (const char *string)
 
 tui_ui_out::tui_ui_out (ui_file *stream)
 : cli_ui_out (stream, 0),
-  m_line (0),
-  m_start_of_line (-1)
+  m_line (-1),
+  m_start_of_line (0)
 {
 }