diff mbox

TUI: Expand TABs into spaces

Message ID 83k3149k5b.fsf@gnu.org
State New
Headers show

Commit Message

Eli Zaretskii Jan. 3, 2015, 11:30 a.m. UTC
"gdb -tui" relies on the curses library and the underlying terminal
driver to expand TAB characters into spaces.  But ncurses on Windows
doesn't do that, and instead displays an IBM graphics character.

The patches below fix that in the command window and in displaying the
registers.

OK to commit?

2015-01-03  Eli Zaretskii  <eliz@gnu.org>

	* tui/tui-regs.c (tui_register_format): Expand TABs into the
	appropriate number of spaces.

	* tui/tui-io.c (tui_puts, tui_redisplay_readline): Expand TABs
	into the appropriate number of spaces.

Comments

Eli Zaretskii Jan. 16, 2015, 11:17 a.m. UTC | #1
Ping!  OK to install, master and 7.9 branch?

> Date: Sat, 03 Jan 2015 13:30:08 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> 
> "gdb -tui" relies on the curses library and the underlying terminal
> driver to expand TAB characters into spaces.  But ncurses on Windows
> doesn't do that, and instead displays an IBM graphics character.
> 
> The patches below fix that in the command window and in displaying the
> registers.
> 
> OK to commit?
> 
> 2015-01-03  Eli Zaretskii  <eliz@gnu.org>
> 
> 	* tui/tui-regs.c (tui_register_format): Expand TABs into the
> 	appropriate number of spaces.
> 
> 	* tui/tui-io.c (tui_puts, tui_redisplay_readline): Expand TABs
> 	into the appropriate number of spaces.
> 
> 
> --- gdb/tui/tui-io.c~0	2014-10-29 21:45:50.000000000 +0200
> +++ gdb/tui/tui-io.c	2015-01-03 11:12:52.187500000 +0200
> @@ -179,7 +179,19 @@ tui_puts (const char *string)
>        else if (tui_skip_line != 1)
>          {
>            tui_skip_line = -1;
> -          waddch (w, c);
> +	  if (c == '\t')
> +	    {
> +	      int line, col;
> +
> +	      getyx (w, line, col);
> +	      do
> +		{
> +		  waddch (w, ' ');
> +		  col++;
> +		} while ((col % 8) != 0);
> +	    }
> +	  else
> +	    waddch (w, c);
>          }
>        else if (c == '\n')
>          tui_skip_line = -1;
> @@ -254,6 +266,15 @@ tui_redisplay_readline (void)
>            waddch (w, '^');
>            waddch (w, CTRL_CHAR (c) ? UNCTRL (c) : '?');
>  	}
> +      else if (c == '\t')
> +	{
> +	  getyx (w, line, col);
> +	  do
> +	    {
> +	      waddch (w, ' ');
> +	      col++;
> +	    } while ((col % 8) != 0);
> +	}
>        else
>  	{
>            waddch (w, c);
> 
> 
> --- gdb/tui/tui-regs.c~0	2014-10-29 21:45:50.000000000 +0200
> +++ gdb/tui/tui-regs.c	2015-01-03 12:52:42.062500000 +0200
> @@ -676,8 +676,9 @@ tui_register_format (struct frame_info *
>    struct ui_file *stream;
>    struct ui_file *old_stdout;
>    struct cleanup *cleanups;
> -  char *p, *s;
> +  char *p, *s, *q;
>    char *ret;
> +  int n_adjust, col;
>  
>    pagination_enabled = 0;
>    old_stdout = gdb_stdout;
> @@ -694,7 +695,47 @@ tui_register_format (struct frame_info *
>    if (s && s[1] == 0)
>      *s = 0;
>  
> -  ret = xstrdup (p);
> +  /* Expand tabs into spaces.  */
> +  /* 1. How many additional characters do we need?  */
> +  for (col = n_adjust = 0, s = p; s; )
> +    {
> +      s = strpbrk (s, "\t");
> +      if (s)
> +	{
> +	  col = (s - p) + n_adjust;
> +	  /* Adjustment for the next tab stop, minus one for the TAB
> +	     we replace with spaces.  */
> +	  n_adjust += 8 - (col % 8) - 1;
> +	  s++;
> +	}
> +    }
> +
> +  /* Allocate the copy.  */
> +  ret = q = xmalloc (strlen (p) + n_adjust + 1);
> +
> +  /* 2. Copy the original string while replacing TABs with spaces.  */
> +  for (col = 0, s = p; s; )
> +    {
> +      char *s1 = strpbrk (s, "\t");
> +      if (s1)
> +	{
> +	  if (s1 > s)
> +	    {
> +	      strncpy (q, s, s1 - s);
> +	      q += s1 - s;
> +	      col += s1 - s;
> +	    }
> +	  do {
> +	    *q++ = ' ';
> +	    col++;
> +	  } while ((col % 8) != 0);
> +	  s1++;
> +	}
> +      else
> +	strcpy (q, s);
> +      s = s1;
> +    }
> +
>    do_cleanups (cleanups);
>  
>    return ret;
>
Doug Evans Jan. 16, 2015, 4:32 p.m. UTC | #2
On Fri, Jan 16, 2015 at 3:17 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> Ping!  OK to install, master and 7.9 branch?
>
>> Date: Sat, 03 Jan 2015 13:30:08 +0200
>> From: Eli Zaretskii <eliz@gnu.org>
>>
>> "gdb -tui" relies on the curses library and the underlying terminal
>> driver to expand TAB characters into spaces.  But ncurses on Windows
>> doesn't do that, and instead displays an IBM graphics character.
>>
>> The patches below fix that in the command window and in displaying the
>> registers.
>>
>> OK to commit?
>>
>> 2015-01-03  Eli Zaretskii  <eliz@gnu.org>
>>
>>       * tui/tui-regs.c (tui_register_format): Expand TABs into the
>>       appropriate number of spaces.
>>
>>       * tui/tui-io.c (tui_puts, tui_redisplay_readline): Expand TABs
>>       into the appropriate number of spaces.

I'd have to read the patch more to say it's ok,
but one thing that is missing are comments
explaining *why* we are expanding tabs into spaces.
Eli Zaretskii Jan. 16, 2015, 4:43 p.m. UTC | #3
> Date: Fri, 16 Jan 2015 08:32:15 -0800
> From: Doug Evans <xdje42@gmail.com>
> Cc: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
> 
> >> 2015-01-03  Eli Zaretskii  <eliz@gnu.org>
> >>
> >>       * tui/tui-regs.c (tui_register_format): Expand TABs into the
> >>       appropriate number of spaces.
> >>
> >>       * tui/tui-io.c (tui_puts, tui_redisplay_readline): Expand TABs
> >>       into the appropriate number of spaces.
> 
> I'd have to read the patch more to say it's ok,
> but one thing that is missing are comments
> explaining *why* we are expanding tabs into spaces.

You mean, the fact that the Windows port of ncurses doesn't?  Sure,
can do that.  But note that a similar feature in displaying the source
doesn't have any such comments.
Doug Evans Jan. 16, 2015, 5:29 p.m. UTC | #4
On Fri, Jan 16, 2015 at 8:43 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>> Date: Fri, 16 Jan 2015 08:32:15 -0800
>> From: Doug Evans <xdje42@gmail.com>
>> Cc: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
>>
>> >> 2015-01-03  Eli Zaretskii  <eliz@gnu.org>
>> >>
>> >>       * tui/tui-regs.c (tui_register_format): Expand TABs into the
>> >>       appropriate number of spaces.
>> >>
>> >>       * tui/tui-io.c (tui_puts, tui_redisplay_readline): Expand TABs
>> >>       into the appropriate number of spaces.
>>
>> I'd have to read the patch more to say it's ok,
>> but one thing that is missing are comments
>> explaining *why* we are expanding tabs into spaces.
>
> You mean, the fact that the Windows port of ncurses doesn't?  Sure,
> can do that.  But note that a similar feature in displaying the source
> doesn't have any such comments.

Is that in relation to the tabset command?
And is that to provide a knob so that source written
with an expectation that \t was different than 8
can be made readable?

Or is there existing code that is coping with these
windows, umm, quirks?
Eli Zaretskii Jan. 16, 2015, 5:53 p.m. UTC | #5
> Date: Fri, 16 Jan 2015 09:29:52 -0800
> From: Doug Evans <xdje42@gmail.com>
> Cc: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
> 
> Is that in relation to the tabset command?
> And is that to provide a knob so that source written
> with an expectation that \t was different than 8
> can be made readable?

No.

> Or is there existing code that is coping with these
> windows, umm, quirks?

Yes.
Doug Evans Jan. 16, 2015, 6:25 p.m. UTC | #6
On Fri, Jan 16, 2015 at 9:53 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>> Date: Fri, 16 Jan 2015 09:29:52 -0800
>> From: Doug Evans <xdje42@gmail.com>
>> Cc: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
>>
>> Is that in relation to the tabset command?
>> And is that to provide a knob so that source written
>> with an expectation that \t was different than 8
>> can be made readable?
>
> No.
>
>> Or is there existing code that is coping with these
>> windows, umm, quirks?
>
> Yes.

Good to know.
Can you point me at it?
Eli Zaretskii Jan. 16, 2015, 8:11 p.m. UTC | #7
> Date: Fri, 16 Jan 2015 10:25:10 -0800
> From: Doug Evans <xdje42@gmail.com>
> Cc: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
> 
> >> Or is there existing code that is coping with these
> >> windows, umm, quirks?
> >
> > Yes.
> 
> Good to know.
> Can you point me at it?

Point you at what?
Doug Evans Jan. 17, 2015, 1:02 a.m. UTC | #8
On Fri, Jan 16, 2015 at 12:11 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>> Date: Fri, 16 Jan 2015 10:25:10 -0800
>> From: Doug Evans <xdje42@gmail.com>
>> Cc: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
>>
>> >> Or is there existing code that is coping with these
>> >> windows, umm, quirks?
>> >
>> > Yes.
>>
>> Good to know.
>> Can you point me at it?
>
> Point you at what?

The "similar feature in displaying the source
doesn't have any such comments."

I thought I found it, but then wasn't sure,
and I don't want to guess.
Eli Zaretskii Jan. 17, 2015, 7:56 a.m. UTC | #9
> Date: Fri, 16 Jan 2015 17:02:27 -0800
> From: Doug Evans <xdje42@gmail.com>
> Cc: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
> 
> >> Can you point me at it?
> >
> > Point you at what?
> 
> The "similar feature in displaying the source
> doesn't have any such comments."

Ah, okay.

tui-source.c:tui_set_source_content, around line 185 of tui-source.c,
is what I had in mind.
Eli Zaretskii Jan. 24, 2015, 12:32 p.m. UTC | #10
Ping! Ping!  OK to install?

> Date: Fri, 16 Jan 2015 08:32:15 -0800
> From: Doug Evans <xdje42@gmail.com>
> Cc: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
> 
> On Fri, Jan 16, 2015 at 3:17 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> > Ping!  OK to install, master and 7.9 branch?
> >
> >> Date: Sat, 03 Jan 2015 13:30:08 +0200
> >> From: Eli Zaretskii <eliz@gnu.org>
> >>
> >> "gdb -tui" relies on the curses library and the underlying terminal
> >> driver to expand TAB characters into spaces.  But ncurses on Windows
> >> doesn't do that, and instead displays an IBM graphics character.
> >>
> >> The patches below fix that in the command window and in displaying the
> >> registers.
> >>
> >> OK to commit?
> >>
> >> 2015-01-03  Eli Zaretskii  <eliz@gnu.org>
> >>
> >>       * tui/tui-regs.c (tui_register_format): Expand TABs into the
> >>       appropriate number of spaces.
> >>
> >>       * tui/tui-io.c (tui_puts, tui_redisplay_readline): Expand TABs
> >>       into the appropriate number of spaces.
> 
> I'd have to read the patch more to say it's ok,
> but one thing that is missing are comments
> explaining *why* we are expanding tabs into spaces.
>
Doug Evans Jan. 24, 2015, 9:26 p.m. UTC | #11
Eli Zaretskii <eliz@gnu.org> writes:
> Ping! Ping!  OK to install?

"Give me a ping, Vasili. One ping only, please."
ref: http://www.imdb.com/title/tt0099810/quotes
[just a little light humor]

>> Date: Fri, 16 Jan 2015 08:32:15 -0800
>> From: Doug Evans <xdje42@gmail.com>
>> Cc: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
>> 
>> On Fri, Jan 16, 2015 at 3:17 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>> > Ping!  OK to install, master and 7.9 branch?
>> >
>> >> Date: Sat, 03 Jan 2015 13:30:08 +0200
>> >> From: Eli Zaretskii <eliz@gnu.org>
>> >>
>> >> "gdb -tui" relies on the curses library and the underlying terminal
>> >> driver to expand TAB characters into spaces.  But ncurses on Windows
>> >> doesn't do that, and instead displays an IBM graphics character.
>> >>
>> >> The patches below fix that in the command window and in displaying the
>> >> registers.
>> >>
>> >> OK to commit?
>> >>
>> >> 2015-01-03  Eli Zaretskii  <eliz@gnu.org>
>> >>
>> >>       * tui/tui-regs.c (tui_register_format): Expand TABs into the
>> >>       appropriate number of spaces.
>> >>
>> >>       * tui/tui-io.c (tui_puts, tui_redisplay_readline): Expand TABs
>> >>       into the appropriate number of spaces.
>> 
>> I'd have to read the patch more to say it's ok,
>> but one thing that is missing are comments
>> explaining *why* we are expanding tabs into spaces.

I check the code (tui-source.c around line 185) and it has this:

                                  else
                                    { /* Store the charcter in the
                                         line buffer.  If it is a tab,
                                         then translate to the correct
                                         number of chars so we don't
                                         overwrite our buffer.  */

So it *does* have a comment explaining why tabs are being expanded.

For tui_puts and tui_redisplay_readline let's add something like
the following:

      /* Windows ncurses doesn't expand tabs, so we have to do that here.  */
      [else] if (c == '\t')

For tui_register_format it used to be such a straightforward function,
it's a shame to grow it by 2x for bitfiddly tab handling.

Let's add a helper routine that takes one string and returns
another with tabs expanded, and put this helper routine in, say tui-io.c
(this routine isn't tui-regs specific), and then call that routine from
tui_register_format.
I didn't review the actual code to do the tab expansion with a microscope.
I'm going to assume it at least mostly works. At least it'll be tucked away.

Also, a note on the ChangeLog:
No blank lines between related entries.

>       * tui/tui-regs.c (tui_register_format): Expand TABs into the
>       appropriate number of spaces.
>
>       * tui/tui-io.c (tui_puts, tui_redisplay_readline): Expand TABs
>       into the appropriate number of spaces.
diff mbox

Patch

--- gdb/tui/tui-io.c~0	2014-10-29 21:45:50.000000000 +0200
+++ gdb/tui/tui-io.c	2015-01-03 11:12:52.187500000 +0200
@@ -179,7 +179,19 @@  tui_puts (const char *string)
       else if (tui_skip_line != 1)
         {
           tui_skip_line = -1;
-          waddch (w, c);
+	  if (c == '\t')
+	    {
+	      int line, col;
+
+	      getyx (w, line, col);
+	      do
+		{
+		  waddch (w, ' ');
+		  col++;
+		} while ((col % 8) != 0);
+	    }
+	  else
+	    waddch (w, c);
         }
       else if (c == '\n')
         tui_skip_line = -1;
@@ -254,6 +266,15 @@  tui_redisplay_readline (void)
           waddch (w, '^');
           waddch (w, CTRL_CHAR (c) ? UNCTRL (c) : '?');
 	}
+      else if (c == '\t')
+	{
+	  getyx (w, line, col);
+	  do
+	    {
+	      waddch (w, ' ');
+	      col++;
+	    } while ((col % 8) != 0);
+	}
       else
 	{
           waddch (w, c);


--- gdb/tui/tui-regs.c~0	2014-10-29 21:45:50.000000000 +0200
+++ gdb/tui/tui-regs.c	2015-01-03 12:52:42.062500000 +0200
@@ -676,8 +676,9 @@  tui_register_format (struct frame_info *
   struct ui_file *stream;
   struct ui_file *old_stdout;
   struct cleanup *cleanups;
-  char *p, *s;
+  char *p, *s, *q;
   char *ret;
+  int n_adjust, col;
 
   pagination_enabled = 0;
   old_stdout = gdb_stdout;
@@ -694,7 +695,47 @@  tui_register_format (struct frame_info *
   if (s && s[1] == 0)
     *s = 0;
 
-  ret = xstrdup (p);
+  /* Expand tabs into spaces.  */
+  /* 1. How many additional characters do we need?  */
+  for (col = n_adjust = 0, s = p; s; )
+    {
+      s = strpbrk (s, "\t");
+      if (s)
+	{
+	  col = (s - p) + n_adjust;
+	  /* Adjustment for the next tab stop, minus one for the TAB
+	     we replace with spaces.  */
+	  n_adjust += 8 - (col % 8) - 1;
+	  s++;
+	}
+    }
+
+  /* Allocate the copy.  */
+  ret = q = xmalloc (strlen (p) + n_adjust + 1);
+
+  /* 2. Copy the original string while replacing TABs with spaces.  */
+  for (col = 0, s = p; s; )
+    {
+      char *s1 = strpbrk (s, "\t");
+      if (s1)
+	{
+	  if (s1 > s)
+	    {
+	      strncpy (q, s, s1 - s);
+	      q += s1 - s;
+	      col += s1 - s;
+	    }
+	  do {
+	    *q++ = ' ';
+	    col++;
+	  } while ((col % 8) != 0);
+	  s1++;
+	}
+      else
+	strcpy (q, s);
+      s = s1;
+    }
+
   do_cleanups (cleanups);
 
   return ret;