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

[Corinna Vinschen]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://cygwin.com/pipermail/cygwin-patches/attachments/20200303/a105a71e/attachment.sig>