Re: [PATCH 1/1] Collect handling of wpixput and wpbuf into a helper class.
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 )
> {
>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
Re: [PATCH 1/1] Collect handling of wpixput and wpbuf into a helper class.
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 )
{
DWORD wn;
WriteConsoleA (handle, buf, ixput, , 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 )
{
WriteConsoleA (handle, buf, ixput, 0, 0);
}
The same would apply to all the other calls of WriteConsoleA, it seems.
Re: [PATCH 1/1] Collect handling of wpixput and wpbuf into a helper class.
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, , 0);
}
else
cursor_get (, );
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, , 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, , 0);
con.state = normal;
wpixput = 0; // <--- Here
So, this might not be worth much...
--
Takashi Yano
Re: [PATCH 1/1] Collect handling of wpixput and wpbuf into a helper class.
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 , DWORD *wn) { WriteConsoleA (handle,
> buf, ixput, wn, 0); };
Camelback, missing space, line too long, obsolete semicolon:
inline void send_out (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
Description: PGP signature
