Re: [PATCH] TUI: Expand TABs into spaces

[Doug Evans <xdje42 at gmail dot com>]

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