[PATCH libX11] Add _XGetRequest as substitute for GetReq/GetReqExtra

[Jamey Sharp]

I pretty much approve whole-heartedly of this patch, but I have a couple
nits to ask that you fix.

On Tue, Oct 25, 2011 at 01:37:56PM +1000, Peter Hutterer wrote:
> Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> ---
>  include/X11/Xlibint.h |   51 ++++++++++++++++--------------------------------
>  src/XlibInt.c         |   26 +++++++++++++++++++++++++
>  2 files changed, 43 insertions(+), 34 deletions(-)
> 
> diff --git a/include/X11/Xlibint.h b/include/X11/Xlibint.h
> index 083498f..2074e96 100644
> --- a/include/X11/Xlibint.h
> +++ b/include/X11/Xlibint.h
> @@ -420,6 +420,19 @@ extern LockInfoPtr _Xglobal_lock;
>  #define WORD64ALIGN
>  #endif /* WORD64 */
>  
> +/**
> + * Return a len-sized request buffer for the request type. This function may
> + * flush the buffer.

Can I suggest "may flush the output queue"? Otherwise it sounds like it
does something to the returned buffer.

> + *
> + * @param dpy The display connection
> + * @param type The request qtype

"request type", right?

> + * @param len Length of the request in bytes
> + *
> + * @returns A pointer to the request buffer with a few default values
> + * initialized.
> + */
> +extern void*
> +_XGetRequest(Display *dpy, CARD8 type, size_t len);

There's almost no precedent in libX11 to derive style guidelines for
"void *" vs. "void*", so there's no local-consistency argument to make;
but I'd prefer a space, both in the declaration here and the function
definition in XlibInt.c.

>  /*
>   * GetReq - Get the next available X request packet in the buffer and
> @@ -432,25 +445,10 @@ extern LockInfoPtr _Xglobal_lock;
>  
>  #if !defined(UNIXCPP) || defined(ANSICPP)
>  #define GetReq(name, req) \
> -        WORD64ALIGN\
> -	if ((dpy->bufptr + SIZEOF(x##name##Req)) > dpy->bufmax)\
> -		_XFlush(dpy);\
> -	req = (x##name##Req *)(dpy->last_req = dpy->bufptr);\
> -	req->reqType = X_##name;\
> -	req->length = (SIZEOF(x##name##Req))>>2;\
> -	dpy->bufptr += SIZEOF(x##name##Req);\
> -	dpy->request++
> -
> +	req = _XGetRequest(dpy, X_##name, SIZEOF(x##name##Req))
>  #else  /* non-ANSI C uses empty comment instead of "##" for token concatenation */
>  #define GetReq(name, req) \
> -        WORD64ALIGN\
> -	if ((dpy->bufptr + SIZEOF(x/**/name/**/Req)) > dpy->bufmax)\
> -		_XFlush(dpy);\
> -	req = (x/**/name/**/Req *)(dpy->last_req = dpy->bufptr);\
> -	req->reqType = X_/**/name;\
> -	req->length = (SIZEOF(x/**/name/**/Req))>>2;\
> -	dpy->bufptr += SIZEOF(x/**/name/**/Req);\
> -	dpy->request++
> +	req = _XGetRequest(dpy, X_/**/name, SIZEOF(x/**/name/**/Req))
>  #endif
>  
>  /* GetReqExtra is the same as GetReq, but allocates "n" additional
> @@ -458,24 +456,10 @@ extern LockInfoPtr _Xglobal_lock;
>  
>  #if !defined(UNIXCPP) || defined(ANSICPP)
>  #define GetReqExtra(name, n, req) \
> -        WORD64ALIGN\
> -	if ((dpy->bufptr + SIZEOF(x##name##Req) + n) > dpy->bufmax)\
> -		_XFlush(dpy);\
> -	req = (x##name##Req *)(dpy->last_req = dpy->bufptr);\
> -	req->reqType = X_##name;\
> -	req->length = (SIZEOF(x##name##Req) + n)>>2;\
> -	dpy->bufptr += SIZEOF(x##name##Req) + n;\
> -	dpy->request++
> +	req = _XGetRequest(dpy, X_##name, SIZEOF(x##name##Req) + n)
>  #else
>  #define GetReqExtra(name, n, req) \
> -        WORD64ALIGN\
> -	if ((dpy->bufptr + SIZEOF(x/**/name/**/Req) + n) > dpy->bufmax)\
> -		_XFlush(dpy);\
> -	req = (x/**/name/**/Req *)(dpy->last_req = dpy->bufptr);\
> -	req->reqType = X_/**/name;\
> -	req->length = (SIZEOF(x/**/name/**/Req) + n)>>2;\
> -	dpy->bufptr += SIZEOF(x/**/name/**/Req) + n;\
> -	dpy->request++
> +	req = _XGetRequest(dpy, X_/**/name, SIZEOF(x/**/name/**/Req) + n)
>  #endif
>  
>  /* GetReqSized is the same as GetReq but allows the caller to specify the

These implementations of GetReq and GetReqExtra look to me like huge
improvements in every way except one: casting to (x##name##Req *) before
assigning to "req" enabled the compiler to check that the argument to
GetReq matched the type of the declared request pointer. I'd put that
cast back on each macro-ized call to _XGetRequest.

GetEmptyReq should also be easy to replace:
	req = (xReq *) _XGetRequest(dpy, X_##name, SIZEOF(xReq))

And I think GetResReq isn't too bad either:
	req = (xResourceReq *) _XGetRequest(dpy, X_##name, SIZEOF(xResourceReq)); \
	req->id = (rid)

I'd like to see all four variants fixed up.

> @@ -503,7 +487,6 @@ extern LockInfoPtr _Xglobal_lock;
>  #endif
>  
>  
> -
>  /*
>   * GetResReq is for those requests that have a resource ID
>   * (Window, Pixmap, GContext, etc.) as their single argument.

This hunk seems a little frivolous. :-)

> diff --git a/src/XlibInt.c b/src/XlibInt.c
> index 3db151e..a143dc4 100644
> --- a/src/XlibInt.c
> +++ b/src/XlibInt.c
> @@ -1956,6 +1956,32 @@ Screen *_XScreenOfWindow(Display *dpy, Window w)
>  }
>  
>  
> +
> +void*
> +_XGetRequest(Display *dpy, CARD8 type, size_t len)

I'd like a big warning comment in here. Something like:

/*
 * WARNING: This implementation's pre-conditions and post-conditions
 * must remain compatible with the old macro-based implementations of
 * GetReq, GetReqExtra, GetResReq, and GetEmptyReq. The portions of the
 * Display structure affected by those macros are part of libX11's ABI.
 */

> +{
> +    xReq *req;
> +
> +    WORD64ALIGN
> +
> +    if (dpy->bufptr + len > dpy->bufmax)
> +	_XFlush(dpy);
> +
> +    if (len % 4)
> +	fprintf(stderr,
> +		"Xlib: request %d length %zd not a multiple of 4.\n",
> +		type, len);
> +
> +    dpy->last_req = dpy->bufptr;
> +
> +    req = (xReq*)dpy->bufptr;
> +    req->reqType = type;
> +    req->length = len/4;

Could I trouble you for spaces around the division operator?

> +    dpy->bufptr += len;
> +    dpy->request++;
> +    return req;
> +}
> +

Generally, though, looks great. Thank you!

Jamey
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.x.org/archives/xorg-devel/attachments/20111025/77c2d4c4/attachment.pgp>