[Xcb] Win32 branch of XCB now up :)

[Jeetu Golani]

Hi Julien,

Thanks so much for your review. Really appreciate it. Just a quick note.

>--- /dev/null
>+++ b/src/windefs.h
>[...]
>+#define WINVER 0x0501 /* required for getaddrinfo/freeaddrinfo defined
only for WinXP and above */

>Hum, are you sure you always want to define it?
>If not, maybe adding #ifndef WINVER / #endif surrounding this #define
>might be a good idea.

Functions such as getaddrinfo and a couple of others are only available
upwards of Win 2000. The code won't compile if Winver is targeted for a
platform which isn't this. I have tested my code primarily under XP though I
don't see why it won't work under 2000 either. For Win95/98,etc I can't say
if it will even compile. I'd like to think this through a little bit.

>You are redefining the whole function. IMHO, this is not so good,
>because you are duplicating a lot of code. And is some day we need to
>fix code in one of that function, we may forget to fix it in the WIN32
>version.

You're right I'll change this asap.

>--- a/src/xcb.h
>+++ b/src/xcb.h
>+int initWSA(void);
>
>This is not right. If I'm understanding correctly, this is required to
>initialize some stuff. The function name is not respecting the naming
>scheme of xcb functions and the name is too generic.

Need to think more on this. Will look into it.

Just playing catch up with some other stuff right now....will look into
these and other changes as suggested by Peter as soon as I can.

Thanks.

Bye for now


On Mon, May 18, 2009 at 1:46 PM, Julien Danjou <julien at danjou.info> wrote:

> HI,
>
> At 1242038193 time_t, Jeetu Golani wrote:
> > I've updated my branch at github to reflect changes in code as suggested
> by
> > Julien and also rectify the goof ups I made with my initial submission.
> >
> > Julien, I would appreciate if you could verify if all is well now.
>
> Yeah, that's much better. :-)
>
> I'll quote some of your work here and just review it. I'm not qualified
> in Windows coding so I won't judge if the work is good, bad or even
> functionnal.
>
> --- /dev/null
> +++ b/src/windefs.h
> [...]
> +#define WINVER 0x0501 /* required for getaddrinfo/freeaddrinfo defined
> only for WinXP and above */
>
> Hum, are you sure you always want to define it?
> If not, maybe adding #ifndef WINVER / #endif surrounding this #define
> might be a good idea.
>
>
> --- a/src/xcb_in.c
> +++ b/src/xcb_in.c
> +static int read_block(const int fd, void *buf, const size_t len)
> and
> +int _xcb_in_read(xcb_connection_t *c)
>
> You are redefining the whole function. IMHO, this is not so good,
> because you are duplicating a lot of code. And is some day we need to
> fix code in one of that function, we may forget to fix it in the WIN32
> version.
>
>
> --- a/src/xcb.h
> +++ b/src/xcb.h
> +int initWSA(void);
>
> This is not right. If I'm understanding correctly, this is required to
> initialize some stuff. The function name is not respecting the naming
> scheme of xcb functions and the name is too generic.
>
> I suggest that:
> 1. You drop the function and documente that loading winsock via
>  WSAStartup() is mandatory before using XCB;
> 2. or you add a static flag in xcb_open() that check that winsock has been
>  initialized before running;
> 3. or you initialize winsock in xcb_open() unconditionnally. I don't know
>  if you can initialize winsock multiple times. If not, the solution
>  above can be better. But again, maybe another lib can initiliaze
>  winsock in its corner, so maybe if it can be initialized only once,
>  solution 1 will be better.
>
> The rest seems fine to me.
>
> Cheers,
> --
> Julien Danjou
> // ᐰ <julien at danjou.info>   http://julien.danjou.info
> // 9A0D 5FD9 EB42 22F6 8974  C95C A462 B51E C2FE E5CD
> // When I get sad, I stop being sad and be awesome instead. True story.
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.9 (GNU/Linux)
>
> iEYEARECAAYFAkoRIF4ACgkQpGK1HsLῄŸ昢ٌ㲃鶳𤫈퍤뎃鱯
> tJsAoNFkKUc1OjxugdpAG9U/sUifmgs6
> =HMAC
> -----END PGP SIGNATURE-----
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.freedesktop.org/archives/xcb/attachments/20090518/eca929b9/attachment.html