dpnet patches

[Stefan Dösinger]

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.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 842 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20140131/59a4ad0e/attachment.sig>