| Message ID | 83k3149k5b.fsf@gnu.org |
|---|---|
| State | New |
| Headers | show |
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; >
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.
> 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.
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?
> 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.
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?
> 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?
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.
> 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.
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. >
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.
--- 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;