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 <[email protected]> > --- > 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
signature.asc
Description: Digital signature
_______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
