[prev in list] [next in list] [prev in thread] [next in thread] 

List:       wine-devel
Subject:    dpnet patches
From:       Stefan_Dösinger <stefandoesinger () gmail ! com>
Date:       2014-01-31 12:39:52
Message-ID: 2236EAFD-73BF-44FE-9D02-35CA111B2B5C () gmail ! com
[Download RAW message or body]

[Attachment #2 (multipart/signed)]


Hi Alistair,

A while ago I promised to have a closer look at your dpnet patches. It’s a bit \
delayed, but here are some comments. How serious are you about dplay? Do you intend \
to do more work towards implementing it, or is that just wishful thinking on my part?

http://source.winehq.org/patches/data/101857:
The entire patch doesn’t do all that much, I think it’s best to hold of on that \
change until the function is meaningfully implemented - unless having a proper return \
value helps you in some way I’m missing.

> +            void *buffer = malloc(256);
Please use HeapAlloc / HeapFree.

http://source.winehq.org/patches/data/101856:

> +DEFINE_GUID(GUID_NULL,0,0,0,0,0,0,0,0,0,0,0);
Isn’t that defined somewhere already? Do you need GUID_NULL to signal an \
uninitialized GUID? Is there a reason why setting the pointer to NULL doesn’t work?

> -IMPORTS   = dpnet ole32
> +IMPORTS   = ole32
I’m not sure about this one, but winetest can automatically skip tests if the tested \
DLL doesn’t exist. It might use the imported libraries for that, or maybe just the \
name of the test. In the former case I think you want to keep the dpnet import.

http://source.winehq.org/patches/data/101855:
http://source.winehq.org/patches/data/101854:
Did you think about sharing code between IDirectPlay8Peer and IDirectPlay8Server? The \
interfaces themselves have no relationship, but their methods are similar.

http://source.winehq.org/patches/data/101853:
> +typedef struct IDirectPlay8ClientImpl
> +{
> +    IDirectPlay8Client IDirectPlay8Client_iface;
> +    LONG ref;
> +} IDirectPlay8ClientImpl;
Are you sure you want to move the definition out of the library-wide header? If that \
works out ok it’s great, but you may need something other than the public interface \
in some other files in this library.

I recommend to work without the typedef, but that should be changed in a separate \
patch.

> -    ULONG refCount = InterlockedIncrement(&This->ref);
> +    ULONG ref = InterlockedIncrement(&This->ref);
> -    TRACE("(%p)->(ref before=%u)\n", This, refCount - 1);
> +    TRACE("(%p) ref=%d\n", This, ref);
ULONG is unsigned, you have to use %u.

http://source.winehq.org/patches/data/101852
> +                    'DPNSPModemModem'
> +                    {
> +                        val 'Friendly Name' = s 'Modem Connection For DirectPlay'
> +                        val 'GUID' = s '{6D4A3650-628D-11D2-AE0F-006097B01411}'
> +                    }
> +
> +                    'DPNSPModemSerial'
> +                    {
> +                        val 'Friendly Name' = s 'Serial Connection For DirectPlay'
> +                        val 'GUID' = s '{743B5D60-628D-11D2-AE0F-006097B01411}'
> +                    }
I know that those always exist on Windows, but wouldn’t it be better to not advertise \
them until we have an actual implementation? I think advertising DPNSPWinsockTCP \
alone should be ok for the start.


["signature.asc" (signature.asc)]

-----BEGIN PGP SIGNATURE-----
Comment: GPGTools - https://gpgtools.org

iQIcBAEBCgAGBQJS65mYAAoJEN0/YqbEcdMwV6YQAIFSNZQcLej5S1yvCa42ecgI
I1gX7xg0K/HU+Q4houa+BoUjNRHmFvhoQ5UN8Htqu7Q/pvJLhyCKcy191JO2+s/H
Zn+nl09jggpXCAdx8q5tS6Pvxkls7RTAMLohJQ8JHzUNhJo01w66kHBhGHPn9trX
SrTaUIdEoicY73UjkBp4k2MZlPbmBnKMmd3YM4WEZZ//qrVyNDz4733mXkuk4KKG
DpoIudNh0UfII88R0TxnDD6wDxLg9J/JFKcP8PKa1UEl99u74FDoADVay1eqHNYr
VARzQY6cm0vcxwXo2vD4DThtRHhvEILvTq0xiqJ1Py1qNQXqaRtBoEhZiiJBIJjc
0a568IKImGrh31y9o/o+JeXv/2Ddzf4/crY6MeAPE/S05lDFnt6qB0KR+UMcC/hk
8Q25qgMPUIrz16ZN+zjkfM/8q4EpNNy8rFva9JXWVSXiwzqnVDi+ZrVgHybJlEub
7rDQpx0ms3JWgagEZ8w+65eN8rOUa4jo3W+bp5mV/gLvPVNSvsuyYXVtUOyiL5Jk
bp24lgmNR8gVGUFEBLI+Kj7n1Bd1gr15OcLMMsQUiRDfkI+aF1nhgFHVHyP3P6/R
7f5FE9u36fowGfCexfSQjjT98MFFl5aogo5zAiWbphwsxUAr89G9aGWPypBlVLDm
FSSSdEEdsfZmMfuif2FM
=sYkW
-----END PGP SIGNATURE-----




[prev in list] [next in list] [prev in thread] [next in thread]