[PATCH] TUI: Expand TABs into spaces

[PATCH] TUI: Expand TABs into spaces

[Eli Zaretskii]

"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;

Re: [PATCH] TUI: Expand TABs into spaces

[Eli Zaretskii]

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

Re: [PATCH] TUI: Expand TABs into spaces

[Doug Evans]

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.

Re: [PATCH] TUI: Expand TABs into spaces

[Eli Zaretskii]

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

Re: [PATCH] TUI: Expand TABs into spaces

[Doug Evans]

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?

Re: [PATCH] TUI: Expand TABs into spaces

[Eli Zaretskii]

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

Re: [PATCH] TUI: Expand TABs into spaces

[Doug Evans]

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?

Re: [PATCH] TUI: Expand TABs into spaces

[Eli Zaretskii]

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

Re: [PATCH] TUI: Expand TABs into spaces

[Doug Evans]

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.

Re: [PATCH] TUI: Expand TABs into spaces

[Eli Zaretskii]

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

Re: [PATCH] TUI: Expand TABs into spaces

[Eli Zaretskii]

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

Re: [PATCH] TUI: Expand TABs into spaces

[Doug Evans]

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.

Re: [PATCH] TUI: Expand TABs into spaces

[Eli Zaretskii]

> From: Doug Evans <xdje42@gmail.com>
> Cc: gdb-patches@sourceware.org
> Date: Sat, 24 Jan 2015 13:26:47 -0800
> 
> 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 did that below, but since tui_register_format is its only user,
keeping that function in tui-regs.c would have allowed us to make it
static.  Wouldn't that be slightly better?

Thanks for the review; updated patch attached.

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

	* tui/tui-io.c (tui_expand_tabs): New function.
	(tui_puts, tui_redisplay_readline): Expand TABs into the
	appropriate number of spaces.
	* tui/tui-regs.c: Include tui-io.h.
	(tui_register_format): Call tui_expand_tabs to expand TABs into
	the appropriate number of spaces.
	* tui/tui-io.h: Add prototype for tui_expand_tabs.

--- tui/tui-io.c~0	Wed Oct 29 21:45:50 2014
+++ tui/tui-io.c	Mon Jan 26 09:10:34 2015
@@ -179,7 +179,20 @@ tui_puts (const char *string)
       else if (tui_skip_line != 1)
         {
           tui_skip_line = -1;
-          waddch (w, c);
+	  /* Expand TABs, since ncurses on MS-Windows doesn't.  */
+	  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 +269,16 @@ tui_redisplay_readline (void)
           waddch (w, '^');
           waddch (w, CTRL_CHAR (c) ? UNCTRL (c) : '?');
 	}
+      else if (c == '\t')
+	{
+	  /* Expand TABs, since ncurses on MS-Windows doesn't.  */
+	  getyx (w, line, col);
+	  do
+	    {
+	      waddch (w, ' ');
+	      col++;
+	    } while ((col % 8) != 0);
+	}
       else
 	{
           waddch (w, c);
@@ -700,6 +725,58 @@ tui_getc (FILE *fp)
   return ch;
 }
 
+/* Utility function to expand TABs in a STRING into spaces.  STRING
+   will be displayed starting at column COL.  The returned expanded
+   string is malloc'ed.  */
+char *
+tui_expand_tabs (const char *string, int col)
+{
+  int n_adjust;
+  const char *s;
+  char *ret, *q;
+
+  /* 1. How many additional characters do we need?  */
+  for (n_adjust = 0, s = string; s; )
+    {
+      s = strpbrk (s, "\t");
+      if (s)
+	{
+	  col += (s - string) + 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 (string) + n_adjust + 1);
+
+  /* 2. Copy the original string while replacing TABs with spaces.  */
+  for (s = string; 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;
+    }
+
+  return ret;
+}
 
 /* Cleanup when a resize has occured.
    Returns the character that must be processed.  */


--- tui/tui-regs.c~0	Wed Oct 29 21:45:50 2014
+++ tui/tui-regs.c	Mon Jan 26 09:24:23 2015
@@ -37,6 +37,7 @@
 #include "tui/tui-wingeneral.h"
 #include "tui/tui-file.h"
 #include "tui/tui-regs.h"
+#include "tui/tui-io.h"
 #include "reggroups.h"
 #include "valprint.h"
 
@@ -694,7 +695,9 @@ tui_register_format (struct frame_info *
   if (s && s[1] == 0)
     *s = 0;
 
-  ret = xstrdup (p);
+  /* Expand tabs into spaces, since ncurses on MS-Windows doesn't.  */
+  ret = tui_expand_tabs (p, 0);
+
   do_cleanups (cleanups);
 
   return ret;


--- tui/tui-io.h~0	Wed Jun 11 19:34:41 2014
+++ tui/tui-io.h	Mon Jan 26 09:11:42 2015
@@ -41,6 +41,9 @@
    changed the edited text.  */
 extern void tui_redisplay_readline (void);
 
+/* Expand TABs into spaces.  */
+extern char *tui_expand_tabs (const char *, int);
+
 extern struct ui_out *tui_out;
 extern struct ui_out *tui_old_uiout;
 

Re: [PATCH] TUI: Expand TABs into spaces

[Doug Evans]

Eli Zaretskii <eliz@gnu.org> writes:
>> From: Doug Evans <xdje42@gmail.com>
>> Cc: gdb-patches@sourceware.org
>> Date: Sat, 24 Jan 2015 13:26:47 -0800
>> 
>> 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 did that below, but since tui_register_format is its only user,
> keeping that function in tui-regs.c would have allowed us to make it
> static.  Wouldn't that be slightly better?

That's not an invalid viewpoint, and if you want to go
that route, fine by me.  One argument for going that route
is that if another user comes along one can always make
it public later.  Alas that doesn't always happen, and one
ends up writing something that already exists.
[If a year from now I need something like that the last
place I'm going to look is tui-regs.c, whereas I will at least look
through all the headers and generic-looking files for something.
I wish I had a previous example at hand, I've paged out the details
from memory and all that's left is wanting to avoid that.
IIRC we've even had cases where something generic was used by
multiple callers, then only one, and then someone moved it and
made it static.  If there were multiple callers once there may
be again (depending on the situation of course) so if I'm given the
choice I wouldn't have made that move.]
[Also, one could argue that this function isn't even tui-specific
and thus should go in something like gdb/utils.c or even libiberty.
Keeping it in tui just violates what I said above about being in
the last place someone would look.
I opted for not getting too carried away with things: I don't
yet see a need for expanding tabs to spaces outside of tui,
but I could be wrong of course.  Plus I wouldn't impose spending
any time going back and forth picking the absolute best location,
there are far more important things, and one could argue I've made
you read too much already.]
[OTOOH :-), if I hadn't said at least some of this the probability
is too high that someone else would.  Bleah.
[Not saying it would be you though!]]

So to make a long story short, feel free to leave it where it is or
make it static.

A few comments below.

> Thanks for the review; updated patch attached.
>
> 2015-01-26  Eli Zaretskii  <eliz@gnu.org>
>
> 	* tui/tui-io.c (tui_expand_tabs): New function.
> 	(tui_puts, tui_redisplay_readline): Expand TABs into the
> 	appropriate number of spaces.
> 	* tui/tui-regs.c: Include tui-io.h.
> 	(tui_register_format): Call tui_expand_tabs to expand TABs into
> 	the appropriate number of spaces.
> 	* tui/tui-io.h: Add prototype for tui_expand_tabs.
>
> --- tui/tui-io.c~0	Wed Oct 29 21:45:50 2014
> +++ tui/tui-io.c	Mon Jan 26 09:10:34 2015
> @@ -179,7 +179,20 @@ tui_puts (const char *string)
>        else if (tui_skip_line != 1)
>          {
>            tui_skip_line = -1;
> -          waddch (w, c);
> +	  /* Expand TABs, since ncurses on MS-Windows doesn't.  */
> +	  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 +269,16 @@ tui_redisplay_readline (void)
>            waddch (w, '^');
>            waddch (w, CTRL_CHAR (c) ? UNCTRL (c) : '?');
>  	}
> +      else if (c == '\t')
> +	{
> +	  /* Expand TABs, since ncurses on MS-Windows doesn't.  */
> +	  getyx (w, line, col);
> +	  do
> +	    {
> +	      waddch (w, ' ');
> +	      col++;
> +	    } while ((col % 8) != 0);
> +	}
>        else
>  	{
>            waddch (w, c);
> @@ -700,6 +725,58 @@ tui_getc (FILE *fp)
>    return ch;
>  }
>  
> +/* Utility function to expand TABs in a STRING into spaces.  STRING
> +   will be displayed starting at column COL.  The returned expanded
> +   string is malloc'ed.  */

blank line between function comment and definition
[I realize tui_get_register doesn't do that, but best do that with
new code.]

Also, I'm guessing that newlines in STRING isn't supported here, right?
If so, mention that in the function comment.
One could even add an assert like:

  gdb_assert (strchr (string, '\n') == NULL);

or some such.

> +char *
> +tui_expand_tabs (const char *string, int col)
> +{
> +  int n_adjust;
> +  const char *s;
> +  char *ret, *q;
> +
> +  /* 1. How many additional characters do we need?  */
> +  for (n_adjust = 0, s = string; s; )
> +    {
> +      s = strpbrk (s, "\t");
> +      if (s)
> +	{
> +	  col += (s - string) + 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 (string) + n_adjust + 1);
> +
> +  /* 2. Copy the original string while replacing TABs with spaces.  */
> +  for (s = string; 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;
> +    }
> +
> +  return ret;
> +}
>  
>  /* Cleanup when a resize has occured.
>     Returns the character that must be processed.  */
>
>
> --- tui/tui-regs.c~0	Wed Oct 29 21:45:50 2014
> +++ tui/tui-regs.c	Mon Jan 26 09:24:23 2015
> @@ -37,6 +37,7 @@
>  #include "tui/tui-wingeneral.h"
>  #include "tui/tui-file.h"
>  #include "tui/tui-regs.h"
> +#include "tui/tui-io.h"
>  #include "reggroups.h"
>  #include "valprint.h"
>  
> @@ -694,7 +695,9 @@ tui_register_format (struct frame_info *
>    if (s && s[1] == 0)
>      *s = 0;
>  
> -  ret = xstrdup (p);
> +  /* Expand tabs into spaces, since ncurses on MS-Windows doesn't.  */
> +  ret = tui_expand_tabs (p, 0);
> +
>    do_cleanups (cleanups);
>  
>    return ret;
>
>
> --- tui/tui-io.h~0	Wed Jun 11 19:34:41 2014
> +++ tui/tui-io.h	Mon Jan 26 09:11:42 2015
> @@ -41,6 +41,9 @@
>     changed the edited text.  */
>  extern void tui_redisplay_readline (void);
>  
> +/* Expand TABs into spaces.  */

For reference sake, there are multiple schools of thought, and no
fixed convention in gdb, for whether to put a comment here, and
what it should look like.  No need to change anything, and I can
go into details if you want.  Just wanted to mention the issue,
and the acceptable choice of not having any comment here.

> +extern char *tui_expand_tabs (const char *, int);
> +
>  extern struct ui_out *tui_out;
>  extern struct ui_out *tui_old_uiout;
>  

Re: [PATCH] TUI: Expand TABs into spaces

[Eli Zaretskii]

> From: Doug Evans <xdje42@gmail.com>
> Cc: gdb-patches@sourceware.org
> Date: Tue, 27 Jan 2015 00:06:12 -0800
> 
> > I did that below, but since tui_register_format is its only user,
> > keeping that function in tui-regs.c would have allowed us to make it
> > static.  Wouldn't that be slightly better?
> 
> That's not an invalid viewpoint, and if you want to go
> that route, fine by me.  One argument for going that route
> is that if another user comes along one can always make
> it public later.  Alas that doesn't always happen, and one
> ends up writing something that already exists.
> [If a year from now I need something like that the last
> place I'm going to look is tui-regs.c, whereas I will at least look
> through all the headers and generic-looking files for something.
> I wish I had a previous example at hand, I've paged out the details
> from memory and all that's left is wanting to avoid that.
> IIRC we've even had cases where something generic was used by
> multiple callers, then only one, and then someone moved it and
> made it static.  If there were multiple callers once there may
> be again (depending on the situation of course) so if I'm given the
> choice I wouldn't have made that move.]
> [Also, one could argue that this function isn't even tui-specific
> and thus should go in something like gdb/utils.c or even libiberty.
> Keeping it in tui just violates what I said above about being in
> the last place someone would look.
> I opted for not getting too carried away with things: I don't
> yet see a need for expanding tabs to spaces outside of tui,
> but I could be wrong of course.  Plus I wouldn't impose spending
> any time going back and forth picking the absolute best location,
> there are far more important things, and one could argue I've made
> you read too much already.]
> [OTOOH :-), if I hadn't said at least some of this the probability
> is too high that someone else would.  Bleah.
> [Not saying it would be you though!]]
> 
> So to make a long story short, feel free to leave it where it is or
> make it static.

I left it where it is, given that the pros and cons are inconclusive.

> > +/* Utility function to expand TABs in a STRING into spaces.  STRING
> > +   will be displayed starting at column COL.  The returned expanded
> > +   string is malloc'ed.  */
> 
> blank line between function comment and definition
> [I realize tui_get_register doesn't do that, but best do that with
> new code.]

Done.

> Also, I'm guessing that newlines in STRING isn't supported here, right?
> If so, mention that in the function comment.

Done.

> One could even add an assert like:
> 
>   gdb_assert (strchr (string, '\n') == NULL);
> 
> or some such.

I didn't do this, I think that'd be too much.  Having a slightly
messed-up screen is far better than having to abort the debug session,
if we ever get to this problem.  The mess-up will most probably be
reported, and we will fix it.

> > +/* Expand TABs into spaces.  */
> 
> For reference sake, there are multiple schools of thought, and no
> fixed convention in gdb, for whether to put a comment here, and
> what it should look like.  No need to change anything, and I can
> go into details if you want.  Just wanted to mention the issue,
> and the acceptable choice of not having any comment here.

I had in the past requests to add such comments, so at least some of
the maintainers favor them.  I left that comment intact.

The patch as I pushed it appears below.  Thanks again for reviewing
it.

commit 312809f8838911dabff84d7ad3ccf341307d2b19
Author: Eli Zaretskii <eliz@gnu.org>
Date:   Sat Jan 31 10:47:14 2015 +0200

    Make sure TABs are expanded in TUI windows on MS-Windows.
    
    gdb/
    2015-01-31  Eli Zaretskii  <eliz@gnu.org>
    
    	* tui/tui-io.c (tui_expand_tabs): New function.
    	(tui_puts, tui_redisplay_readline): Expand TABs into the
    	appropriate number of spaces.
    	* tui/tui-regs.c: Include tui-io.h.
    	(tui_register_format): Call tui_expand_tabs to expand TABs into
    	the appropriate number of spaces.
    	* tui/tui-io.h: Add prototype for tui_expand_tabs.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 58f5a7b..51f1714 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,13 @@
+2015-01-31  Eli Zaretskii  <eliz@gnu.org>
+
+	* tui/tui-io.c (tui_expand_tabs): New function.
+	(tui_puts, tui_redisplay_readline): Expand TABs into the
+	appropriate number of spaces.
+	* tui/tui-regs.c: Include tui-io.h.
+	(tui_register_format): Call tui_expand_tabs to expand TABs into
+	the appropriate number of spaces.
+	* tui/tui-io.h: Add prototype for tui_expand_tabs.
+
 2015-01-30  Doug Evans  <dje@google.com>
 
 	* NEWS: "info source" command now display producer string if present.
diff --git a/gdb/tui/tui-io.c b/gdb/tui/tui-io.c
index 7e8a3bc..19e9485 100644
--- a/gdb/tui/tui-io.c
+++ b/gdb/tui/tui-io.c
@@ -178,7 +178,20 @@
       else if (tui_skip_line != 1)
         {
           tui_skip_line = -1;
-          waddch (w, c);
+	  /* Expand TABs, since ncurses on MS-Windows doesn't.  */
+	  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;
@@ -256,6 +269,16 @@
           waddch (w, '^');
           waddch (w, CTRL_CHAR (c) ? UNCTRL (c) : '?');
 	}
+      else if (c == '\t')
+	{
+	  /* Expand TABs, since ncurses on MS-Windows doesn't.  */
+	  getyx (w, line, col);
+	  do
+	    {
+	      waddch (w, ' ');
+	      col++;
+	    } while ((col % 8) != 0);
+	}
       else
 	{
           waddch (w, c);
@@ -724,6 +747,59 @@
   return ch;
 }
 
+/* Utility function to expand TABs in a STRING into spaces.  STRING
+   will be displayed starting at column COL, and is assumed to include
+   no newlines.  The returned expanded string is malloc'ed.  */
+
+char *
+tui_expand_tabs (const char *string, int col)
+{
+  int n_adjust;
+  const char *s;
+  char *ret, *q;
+
+  /* 1. How many additional characters do we need?  */
+  for (n_adjust = 0, s = string; s; )
+    {
+      s = strpbrk (s, "\t");
+      if (s)
+	{
+	  col += (s - string) + 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 (string) + n_adjust + 1);
+
+  /* 2. Copy the original string while replacing TABs with spaces.  */
+  for (s = string; 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;
+    }
+
+  return ret;
+}
 
 /* Cleanup when a resize has occured.
    Returns the character that must be processed.  */
diff --git a/gdb/tui/tui-io.h b/gdb/tui/tui-io.h
index 8f96cfe..3154eee 100644
--- a/gdb/tui/tui-io.h
+++ b/gdb/tui/tui-io.h
@@ -41,6 +41,9 @@
    changed the edited text.  */
 extern void tui_redisplay_readline (void);
 
+/* Expand TABs into spaces.  */
+extern char *tui_expand_tabs (const char *, int);
+
 extern struct ui_out *tui_out;
 extern struct ui_out *tui_old_uiout;
 
diff --git a/gdb/tui/tui-regs.c b/gdb/tui/tui-regs.c
index b523e90..961491f 100644
--- a/gdb/tui/tui-regs.c
+++ b/gdb/tui/tui-regs.c
@@ -36,6 +36,7 @@
 #include "tui/tui-wingeneral.h"
 #include "tui/tui-file.h"
 #include "tui/tui-regs.h"
+#include "tui/tui-io.h"
 #include "reggroups.h"
 #include "valprint.h"
 
@@ -693,7 +694,9 @@ static enum tui_status tui_get_register (struct frame_info *frame,
   if (s && s[1] == 0)
     *s = 0;
 
-  ret = xstrdup (p);
+  /* Expand tabs into spaces, since ncurses on MS-Windows doesn't.  */
+  ret = tui_expand_tabs (p, 0);
+
   do_cleanups (cleanups);
 
   return ret;