[PATCH 1/1] Collect handling of wpixput and wpbuf into a helper class.

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH 1/1] Collect handling of wpixput and wpbuf into a helper class.

Hans-Bernhard Bröker
Replace direct access to a pair of co-dependent variables
by calls to methods of a class that encapsulates their relation.

Also replace C #define by C++ class constant.
---
  winsup/cygwin/fhandler_console.cc | 135 ++++++++++++++++--------------
  1 file changed, 70 insertions(+), 65 deletions(-)

diff --git a/winsup/cygwin/fhandler_console.cc
b/winsup/cygwin/fhandler_console.cc
index c5f269168..af2fb11a4 100644
--- a/winsup/cygwin/fhandler_console.cc
+++ b/winsup/cygwin/fhandler_console.cc
@@ -59,17 +59,22 @@ static struct fhandler_base::rabuf_t con_ra;
   /* Write pending buffer for ESC sequence handling
     in xterm compatible mode */
-#define WPBUF_LEN 256
-static unsigned char wpbuf[WPBUF_LEN];
-static int wpixput;
  static unsigned char last_char;
  -static inline void
-wpbuf_put (unsigned char x)
+// simple helper class to accumulate output in a buffer
+// and send that to the console on request:
+static class WritePendingBuf
  {
-  if (wpixput < WPBUF_LEN)
-    wpbuf[wpixput++] = x;
-}
+private:
+  static const size_t WPBUF_LEN = 256u;
+  unsigned char buf[WPBUF_LEN];
+  size_t ixput;
+
+public:
+  inline void put(unsigned char x) { if (ixput < WPBUF_LEN) {
buf[ixput++] = x; } };
+  inline void empty() { ixput = 0u; };
+  inline void sendOut(HANDLE &handle, DWORD *wn) { WriteConsoleA
(handle, buf, ixput, wn, 0); };
+} wpbuf;
   static void
  beep ()
@@ -2030,10 +2035,10 @@ fhandler_console::char_command (char c)
   break;
  #endif
  case 'b': /* REP */
-  wpbuf_put (c);
+  wpbuf.put (c);
   if (wincap.has_con_esc_rep ())
     /* Just send the sequence */
-    WriteConsoleA (get_output_handle (), wpbuf, wpixput, &wn, 0);
+    wpbuf.sendOut( get_output_handle (), &wn);
   else if (last_char && last_char != '\n')
     for (int i = 0; i < con.args[0]; i++)
       WriteConsoleA (get_output_handle (), &last_char, 1, &wn, 0);
@@ -2041,9 +2046,9 @@ fhandler_console::char_command (char c)
  case 'r': /* DECSTBM */
   con.scroll_region.Top = con.args[0] ? con.args[0] - 1 : 0;
   con.scroll_region.Bottom = con.args[1] ? con.args[1] - 1 : -1;
-  wpbuf_put (c);
+  wpbuf.put (c);
   /* Just send the sequence */
-  WriteConsoleA (get_output_handle (), wpbuf, wpixput, &wn, 0);
+  wpbuf.sendOut( get_output_handle (), &wn);
   break;
  case 'L': /* IL */
   if (wincap.has_con_broken_il_dl ())
@@ -2067,8 +2072,8 @@ fhandler_console::char_command (char c)
        y + 1 - con.b.srWindow.Top,
        srBottom + 1 - con.b.srWindow.Top);
       WriteConsoleA (get_output_handle (), buf, strlen (buf), &wn, 0);
-      wpbuf_put ('T');
-      WriteConsoleA (get_output_handle (), wpbuf, wpixput, &wn, 0);
+      wpbuf.put ('T');
+      wpbuf.sendOut( get_output_handle (), &wn);
       __small_sprintf (buf, "\033[%d;%dr",
        srTop + 1 - con.b.srWindow.Top,
        srBottom + 1 - con.b.srWindow.Top);
@@ -2079,9 +2084,9 @@ fhandler_console::char_command (char c)
     }
   else
     {
-      wpbuf_put (c);
+      wpbuf.put (c);
       /* Just send the sequence */
-      WriteConsoleA (get_output_handle (), wpbuf, wpixput, &wn, 0);
+      wpbuf.sendOut( get_output_handle (), &wn);
     }
   break;
  case 'M': /* DL */
@@ -2095,8 +2100,8 @@ fhandler_console::char_command (char c)
        y + 1 - con.b.srWindow.Top,
        srBottom + 1 - con.b.srWindow.Top);
       WriteConsoleA (get_output_handle (), buf, strlen (buf), &wn, 0);
-      wpbuf_put ('S');
-      WriteConsoleA (get_output_handle (), wpbuf, wpixput, &wn, 0);
+      wpbuf.put ('S');
+      wpbuf.sendOut( get_output_handle (), &wn);
       __small_sprintf (buf, "\033[%d;%dr",
        srTop + 1 - con.b.srWindow.Top,
        srBottom + 1 - con.b.srWindow.Top);
@@ -2107,13 +2112,13 @@ fhandler_console::char_command (char c)
     }
   else
     {
-      wpbuf_put (c);
+      wpbuf.put (c);
       /* Just send the sequence */
-      WriteConsoleA (get_output_handle (), wpbuf, wpixput, &wn, 0);
+      wpbuf.sendOut( get_output_handle (), &wn);
     }
   break;
  case 'J': /* ED */
-  wpbuf_put (c);
+  wpbuf.put (c);
   if (con.args[0] == 3 && wincap.has_con_broken_csi3j ())
     { /* Workaround for broken CSI3J in Win10 1809 */
       CONSOLE_SCREEN_BUFFER_INFO sbi;
@@ -2131,7 +2136,7 @@ fhandler_console::char_command (char c)
     }
   else
     /* Just send the sequence */
-    WriteConsoleA (get_output_handle (), wpbuf, wpixput, &wn, 0);
+    wpbuf.sendOut( get_output_handle (), &wn);
   break;
  case 'h': /* DECSET */
  case 'l': /* DECRST */
@@ -2139,9 +2144,9 @@ fhandler_console::char_command (char c)
     con.screen_alternated = true;
   else
     con.screen_alternated = false;
-  wpbuf_put (c);
+  wpbuf.put (c);
   /* Just send the sequence */
-  WriteConsoleA (get_output_handle (), wpbuf, wpixput, &wn, 0);
+  wpbuf.sendOut( get_output_handle (), &wn);
   if (con.saw_question_mark)
     {
       bool need_fix_tab_position = false;
@@ -2159,15 +2164,15 @@ fhandler_console::char_command (char c)
       con.scroll_region.Top = 0;
       con.scroll_region.Bottom = -1;
     }
-  wpbuf_put (c);
+  wpbuf.put (c);
   /* Just send the sequence */
-  WriteConsoleA (get_output_handle (), wpbuf, wpixput, &wn, 0);
+  wpbuf.sendOut( get_output_handle (), &wn);
   break;
  default:
   /* Other escape sequences */
-  wpbuf_put (c);
+  wpbuf.put (c);
   /* Just send the sequence */
-  WriteConsoleA (get_output_handle (), wpbuf, wpixput, &wn, 0);
+  wpbuf.sendOut( get_output_handle (), &wn);
   break;
  }
        return;
@@ -2874,7 +2879,7 @@ do_print:
   break;
  case ESC:
   con.state = gotesc;
-  wpbuf_put (*found);
+  wpbuf.put (*found);
   break;
  case DWN:
   cursor_get (&x, &y);
@@ -2989,7 +2994,7 @@ fhandler_console::write (const void *vsrc, size_t len)
  case gotesc:
   if (*src == '[') /* CSI Control Sequence Introducer */
     {
-      wpbuf_put (*src);
+      wpbuf.put (*src);
       con.state = gotsquare;
       memset (con.args, 0, sizeof con.args);
       con.nargs = 0;
@@ -3005,13 +3010,13 @@ fhandler_console::write (const void *vsrc,
size_t len)
   /* For xterm mode only */
   DWORD n;
   /* Just send the sequence */
-  wpbuf_put (*src);
-  WriteConsoleA (get_output_handle (), wpbuf, wpixput, &n, 0);
+  wpbuf.put (*src);
+  wpbuf.sendOut( get_output_handle (), &n);
  }
       else if (con.savex >= 0 && con.savey >= 0)
  cursor_set (false, con.savex, con.savey);
       con.state = normal;
-      wpixput = 0;
+      wpbuf.empty();
     }
   else if (*src == '7') /* DECSC Save cursor position */
     {
@@ -3020,13 +3025,13 @@ fhandler_console::write (const void *vsrc,
size_t len)
   /* For xterm mode only */
   DWORD n;
   /* Just send the sequence */
-  wpbuf_put (*src);
-  WriteConsoleA (get_output_handle (), wpbuf, wpixput, &n, 0);
+  wpbuf.put (*src);
+  wpbuf.sendOut( get_output_handle (), &n);
  }
       else
  cursor_get (&con.savex, &con.savey);
       con.state = normal;
-      wpixput = 0;
+      wpbuf.empty();
     }
   else if (wincap.has_con_24bit_colors () && !con_is_legacy
    && wincap.has_con_broken_il_dl () && *src == 'M')
@@ -3048,14 +3053,14 @@ fhandler_console::write (const void *vsrc,
size_t len)
      buf, strlen (buf), &n, 0);
     }
   /* Substitute "CSI Ps T" */
-  wpbuf_put ('[');
-  wpbuf_put ('T');
+  wpbuf.put ('[');
+  wpbuf.put ('T');
  }
       else
- wpbuf_put (*src);
-      WriteConsoleA (get_output_handle (), wpbuf, wpixput, &n, 0);
+ wpbuf.put (*src);
+      wpbuf.sendOut( get_output_handle (), &n);
       con.state = normal;
-      wpixput = 0;
+      wpbuf.empty();
     }
   else if (wincap.has_con_24bit_colors () && !con_is_legacy)
     {
@@ -3067,28 +3072,28 @@ fhandler_console::write (const void *vsrc,
size_t len)
       /* ESC sequences below (e.g. OSC, etc) are left to xterm
  emulation in xterm compatible mode, therefore, are not
  handled and just sent them. */
-      wpbuf_put (*src);
+      wpbuf.put (*src);
       /* Just send the sequence */
       DWORD n;
-      WriteConsoleA (get_output_handle (), wpbuf, wpixput, &n, 0);
+      wpbuf.sendOut( get_output_handle (), &n);
       con.state = normal;
-      wpixput = 0;
+      wpbuf.empty();
     }
   else if (*src == ']') /* OSC Operating System Command */
     {
-      wpbuf_put (*src);
+      wpbuf.put (*src);
       con.rarg = 0;
       con.my_title_buf[0] = '\0';
       con.state = gotrsquare;
     }
   else if (*src == '(') /* Designate G0 character set */
     {
-      wpbuf_put (*src);
+      wpbuf.put (*src);
       con.state = gotparen;
     }
   else if (*src == ')') /* Designate G1 character set */
     {
-      wpbuf_put (*src);
+      wpbuf.put (*src);
       con.state = gotrparen;
     }
   else if (*src == 'M') /* Reverse Index (scroll down) */
@@ -3096,7 +3101,7 @@ fhandler_console::write (const void *vsrc, size_t len)
       con.fillin (get_output_handle ());
       scroll_buffer_screen (0, 0, -1, -1, 0, 1);
       con.state = normal;
-      wpixput = 0;
+      wpbuf.empty();
     }
   else if (*src == 'c') /* RIS Full Reset */
     {
@@ -3107,17 +3112,17 @@ fhandler_console::write (const void *vsrc,
size_t len)
       cursor_set (false, 0, 0);
       clear_screen (cl_buf_beg, cl_buf_beg, cl_buf_end, cl_buf_end);
       con.state = normal;
-      wpixput = 0;
+      wpbuf.empty();
     }
   else if (*src == 'R') /* ? */
     {
       con.state = normal;
-      wpixput = 0;
+      wpbuf.empty();
     }
   else
     {
       con.state = normal;
-      wpixput = 0;
+      wpbuf.empty();
     }
   src++;
   break;
@@ -3126,19 +3131,19 @@ fhandler_console::write (const void *vsrc,
size_t len)
     {
       if (con.nargs < MAXARGS)
  con.args[con.nargs] = con.args[con.nargs] * 10 + *src - '0';
-      wpbuf_put (*src);
+      wpbuf.put (*src);
       src++;
     }
   else if (*src == ';')
     {
-      wpbuf_put (*src);
+      wpbuf.put (*src);
       src++;
       if (con.nargs < MAXARGS)
  con.nargs++;
     }
   else if (*src == ' ')
     {
-      wpbuf_put (*src);
+      wpbuf.put (*src);
       src++;
       con.saw_space = true;
       con.state = gotcommand;
@@ -3151,7 +3156,7 @@ fhandler_console::write (const void *vsrc, size_t len)
     con.nargs++;
   char_command (*src++);
   con.state = normal;
-  wpixput = 0;
+  wpbuf.empty();
   break;
  case gotrsquare:
   if (isdigit (*src))
@@ -3162,7 +3167,7 @@ fhandler_console::write (const void *vsrc, size_t len)
     con.state = eatpalette;
   else
     con.state = eattitle;
-  wpbuf_put (*src);
+  wpbuf.put (*src);
   src++;
   break;
  case eattitle:
@@ -3174,13 +3179,13 @@ fhandler_console::write (const void *vsrc,
size_t len)
  if (*src == '\007' && con.state == gettitle)
   set_console_title (con.my_title_buf);
  con.state = normal;
- wpixput = 0;
+ wpbuf.empty();
       }
     else if (n < TITLESIZE)
       {
  con.my_title_buf[n++] = *src;
  con.my_title_buf[n] = '\0';
- wpbuf_put (*src);
+ wpbuf.put (*src);
       }
     src++;
     break;
@@ -3188,13 +3193,13 @@ fhandler_console::write (const void *vsrc,
size_t len)
  case eatpalette:
   if (*src == '\033')
     {
-      wpbuf_put (*src);
+      wpbuf.put (*src);
       con.state = endpalette;
     }
   else if (*src == '\a')
     {
       con.state = normal;
-      wpixput = 0;
+      wpbuf.empty();
     }
   src++;
   break;
@@ -3204,14 +3209,14 @@ fhandler_console::write (const void *vsrc,
size_t len)
   else
     /* Sequence error (abort) */
     con.state = normal;
-  wpixput = 0;
+  wpbuf.empty();
   src++;
   break;
  case gotsquare:
   if (*src == ';')
     {
       con.state = gotarg1;
-      wpbuf_put (*src);
+      wpbuf.put (*src);
       if (con.nargs < MAXARGS)
  con.nargs++;
       src++;
@@ -3226,7 +3231,7 @@ fhandler_console::write (const void *vsrc, size_t len)
  con.saw_greater_than_sign = true;
       else if (*src == '!')
  con.saw_exclamation_mark = true;
-      wpbuf_put (*src);
+      wpbuf.put (*src);
       /* ignore any extra chars between [ and first arg or command */
       src++;
     }
@@ -3239,7 +3244,7 @@ fhandler_console::write (const void *vsrc, size_t len)
   else
     con.vt100_graphics_mode_G0 = false;
   con.state = normal;
-  wpixput = 0;
+  wpbuf.empty();
   src++;
   break;
  case gotrparen: /* Designate G1 Character Set (ISO 2022) */
@@ -3248,7 +3253,7 @@ fhandler_console::write (const void *vsrc, size_t len)
   else
     con.vt100_graphics_mode_G1 = false;
   con.state = normal;
-  wpixput = 0;
+  wpbuf.empty();
   src++;
   break;
  }
--
2.21.0

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/1] Collect handling of wpixput and wpbuf into a helper class.

Takashi Yano
Hi Hans,

Thanks for the patch.

On Tue, 3 Mar 2020 00:07:25 +0100
Hans-Bernhard Bröker wrote:
> +  inline void sendOut(HANDLE &handle, DWORD *wn) { WriteConsoleA
> (handle, buf, ixput, wn, 0); };

The second argument DWORD *wn of sendOut() is not used
outside sendOut(), so it can be covered up like:

inline void sendOut (HANDLE &handle)
{
  DWORD wn;
  WriteConsoleA (handle, buf, ixput, &wn, 0);
}

--
Takashi Yano <[hidden email]>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/1] Collect handling of wpixput and wpbuf into a helper class.

Corinna Vinschen-2
In reply to this post by Hans-Bernhard Bröker
Hi Hans-Bernhard,

On Mar  3 00:07, Hans-Bernhard Bröker wrote:

> Replace direct access to a pair of co-dependent variables
> by calls to methods of a class that encapsulates their relation.
>
> Also replace C #define by C++ class constant.
> ---
>  winsup/cygwin/fhandler_console.cc | 135 ++++++++++++++++--------------
>  1 file changed, 70 insertions(+), 65 deletions(-)
>
> diff --git a/winsup/cygwin/fhandler_console.cc
> b/winsup/cygwin/fhandler_console.cc
> index c5f269168..af2fb11a4 100644
> --- a/winsup/cygwin/fhandler_console.cc
> +++ b/winsup/cygwin/fhandler_console.cc
> @@ -59,17 +59,22 @@ static struct fhandler_base::rabuf_t con_ra;
>   /* Write pending buffer for ESC sequence handling
>     in xterm compatible mode */
> -#define WPBUF_LEN 256
> -static unsigned char wpbuf[WPBUF_LEN];
> -static int wpixput;
>  static unsigned char last_char;
>  -static inline void
> -wpbuf_put (unsigned char x)
This patch won't apply since commit ecf27dd2e0ed.  Can you please
recreate the patch on top of current master?

Also, a few style issues:

> +// simple helper class to accumulate output in a buffer
> +// and send that to the console on request:

The /* */ style of comments is preferred.  Please use it always
for multiline comments.

> +static class WritePendingBuf

No camelback, please.  Make that `static class write_pending_buf'.

>  {
> -  if (wpixput < WPBUF_LEN)
> -    wpbuf[wpixput++] = x;
> -}
> +private:
> +  static const size_t WPBUF_LEN = 256u;
> +  unsigned char buf[WPBUF_LEN];
> +  size_t ixput;
> +
> +public:
> +  inline void put(unsigned char x) { if (ixput < WPBUF_LEN) { buf[ixput++]
> = x; } };
Please put a space before an opening parenthesis, i.e.

  inline void put (...)

The semicolon after the closing brace is obsolete.

Line length > 80 chars.  Also, it's an expression which is multiline
by default.  Make that

  inline void put (unsigned char x)
  {
    if (ixput < WPBUF_LEN)
      buf[ixput++] = x;
  }

> +  inline void empty() { ixput = 0u; };
> +  inline void sendOut(HANDLE &handle, DWORD *wn) { WriteConsoleA (handle,
> buf, ixput, wn, 0); };

Camelback, missing space, line too long, obsolete semicolon:

  inline void send_out (HANDLE &handle, DWORD *wn)
  { WriteConsoleA (handle, buf, ixput, wn, 0); }

"send" or "write" or "flush" would be ok as name, too, no underscore :)

Btw., looking through the code with this change I wonder about ixput not
being set to 0 in sendOut, right after calling WriteConsoleA.  That
would drop the need to call empty after calls to sendOut and thus clean
up the code, no?


Thanks,
Corinna

--
Corinna Vinschen
Cygwin Maintainer

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/1] Collect handling of wpixput and wpbuf into a helper class.

Takashi Yano
On Tue, 3 Mar 2020 12:14:00 +0100
Corinna Vinschen wrote:
> Btw., looking through the code with this change I wonder about ixput not
> being set to 0 in sendOut, right after calling WriteConsoleA.  That
> would drop the need to call empty after calls to sendOut and thus clean
> up the code, no?

This sounds reasonable. However, for the current console code,
most of wpixput = 0 can not be omitted by this.

For example,
          else if (*src == '7')         /* DECSC Save cursor position */
            {
              if (con.screen_alternated)
                {
                  /* For xterm mode only */
                  DWORD n;
                  /* Just send the sequence */
                  wpbuf_put (*src);
                  WriteConsoleA (get_output_handle (), wpbuf, wpixput, &n, 0);
                }
              else
                cursor_get (&con.savex, &con.savey);
              con.state = normal;
              wpixput = 0;  // <--- This can not be omitted.
            }

This can drop only two wpixput = 0.

                  /* Substitute "CSI Ps T" */
                  wpbuf_put ('[');
                  wpbuf_put ('T');
                }
              else
                wpbuf_put (*src);
              WriteConsoleA (get_output_handle (), wpbuf, wpixput, &n, 0);
              con.state = normal;
              wpixput = 0; // <--- Here
[...]
              /* ESC sequences below (e.g. OSC, etc) are left to xterm
                 emulation in xterm compatible mode, therefore, are not
                 handled and just sent them. */
              wpbuf_put (*src);
              /* Just send the sequence */
              DWORD n;
              WriteConsoleA (get_output_handle (), wpbuf, wpixput, &n, 0);
              con.state = normal;
              wpixput = 0; // <--- Here

So, this might not be worth much...

--
Takashi Yano <[hidden email]>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/1] Collect handling of wpixput and wpbuf into a helper class.

Hans-Bernhard Bröker
In reply to this post by Takashi Yano
Am 03.03.2020 um 01:35 schrieb Takashi Yano:

> The second argument DWORD *wn of sendOut() is not used
> outside sendOut(), so it can be covered up like:
>
> inline void sendOut (HANDLE &handle)
> {
>    DWORD wn;
>    WriteConsoleA (handle, buf, ixput, &wn, 0);
> }
>

I doubt that will improve much, if anything.  There are still direct
calls to WriteConsoleA() left, working on other buffers, and those still
use the DWORD wn defined near the top of
fhandler_console::char_command().  So that the existing varialbe would
have to be kept anyway.  That means the variables local to each
invocation (!) of wpbuf.sendOut would just clutter the stack for no gain.

OTOH the MS documentation calls this DWORD* an "optional output"
argument.  If I'm reading that right, it means it should be fine to just
pass NULL to indicate that we don't need it:

inline void sendOut (HANDLE &handle)
{
   WriteConsoleA (handle, buf, ixput, 0, 0);
}

The same would apply to all the other calls of WriteConsoleA, it seems.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/1] Collect handling of wpixput and wpbuf into a helper class.

Takashi Yano
On Tue, 3 Mar 2020 21:03:38 +0100
Hans-Bernhard Bröker wrote:

> OTOH the MS documentation calls this DWORD* an "optional output"
> argument.  If I'm reading that right, it means it should be fine to just
> pass NULL to indicate that we don't need it:
>
> inline void sendOut (HANDLE &handle)
> {
>    WriteConsoleA (handle, buf, ixput, 0, 0);
> }
>
> The same would apply to all the other calls of WriteConsoleA, it seems.

Yeah, it could be. However, please note that it should be
saparate patch if you remove wn from WriteConsoleA() other
than wpbuf related.

--
Takashi Yano <[hidden email]>