[Xcb] Win32 port - pls review

[Jamey Sharp]

I promised I'd review, but I think others have covered pretty much
everything I care about. :-) As Peter says, it's more or less down to
nit-picking.

It doesn't seem right to me to '#include "xcb_windefs.h"' in xcb.h. If
it needs to be a public header file (and it looks like it does) then
that should be <xcb/windefs.h> or something, except that won't work when
compiling XCB itself. I think I'd actually prefer to place the
Windows-specific definitions directly in xcb.h. Anyone else have an
opinion?

Looks like _xcb_open on Win32 could fall off the end without defining a
return value. Have you compiled this with warnings turned on?

Please do unconditionally replace read with recv, as others suggested.

I think you could simplify the write_vec changes by providing your own
function named 'writev', and removing the #ifdefs from write_vec. What
do you think?

How embarrassing. I just caught myself wishing for Xtrans.

On Wed, Mar 31, 2010 at 09:51:33AM -0700, Barton C Massey wrote:
> In message <201003312215.43723.jeetu.golani at gmail.com> you wrote:
> > a) xcb should initialize errno to 0 before calling select for all it's 
> > variants - *ix  and Win32.
> 
> You often want (a) for the *ix case anyway; I'd just go
> there.

Fine with me, but I thought there were platforms where you can't assign
to errno--in fact, I thought Win32 was one of those platforms?

On Tue, Mar 30, 2010 at 05:25:33PM -0400, Peter Harris wrote:
> SOCKET on Win32 is currently defined to intptr_t, which is wider than
> int on 64-bit systems. We can probably get away with leaving fd as an
> int, for the sake of keeping the code at least somewhat sane. (Neither
> breaking the ABI on *ix nor #ifdefing the type on Win32 are particularly
> appealing, from what I can see).

Anyone have an opinion on typedef'ing some new name, xcb_socket_t or
something, and using that anywhere we pass an fd around in XCB? It
wouldn't bother me too much, but I suppose it could be upsetting to
somebody. :-)

> There's trailing whitespace in a few places. "git diff --color" to see them.

Or `git diff --check`, which will tell you exact file and line number.

> _XCB_WINDEFS_H -- identifiers starting with underscore followed by
> another underscore or a capital letter are reserved in C (7.1.3). I've
> already lost this battle, as the rest of xcb already stomps on this
> namespace, but I thought I'd point it out.
> 
> What does the rest of the list think? Is it better to use a name that
> resembles the rest of the names in XCB, or should we try to avoid the
> reserved namespace going forward?

I'd be happy to see a patch that removes the leading underscores from
the include-once #defines. It's not like that's part of XCB's API.

I'm sad to hear that _t typenames are reserved, since we can't change
those now.

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.freedesktop.org/archives/xcb/attachments/20100331/a8ce2d8d/attachment.pgp>