[dplayx 02/29] Tests for DirectPlayCreate

Michael Karcher wine at mkarcher.dialup.fu-berlin.de
Wed Jul 9 11:21:21 CDT 2008


Hello Ismael,

reviewing this patch shows some issues.

> +#define _okHR(expected, result)                         \
> +    ok( (expected) == (result),                         \
> +        "expected=%d(%s) got=%d(%s)\n",                 \
> +        HRESULT_CODE(expected), dpResult2str(expected), \
> +        HRESULT_CODE(result), dpResult2str(result) );
I dislike the order of the parameters and the name. You would write
 ok( hr == DP_OK, "Something failed with %x\n", hr);
In this line, "hr" is before "DP_OK". So as an analogy, I also want to
write
  _okHR( hr, DD_OK );

Why do you start the macro name with an underscore? And last but not
least: As I see it, a static inline would do the job too, and is always
preferrable to macros.

> +    heap = HeapCreate( 0, 0, 0 );
Do you have any reason to create your own heap? Each windows process
already comes equipped with a standard heap you can obtain using the API
function GetProcessHeap, so this line:
> +    IUnknown* pUnk = HeapAlloc( heap, 0, sizeof(LPDWORD) );
would then look like:
+    IUnknown* pUnk = HeapAlloc( GetProcessHeap(), 0, sizeof(LPDWORD) );

Another point is that this line is totally wrong anyway. An IUnknown* is
a pointer to an COM object, which starts with a vtable pointer. You let
pUnk point to uninitialized memory. While it doesn't matter in this case
(as DirectPlayCreate will never access what the pointer points to), it
is really bad style. I understand the intention to test that aggregation
is not supported for DirectPlay, but you still should keep the contract
of the creation function that pUnk either is NULL or points to a valid
COM object. A basic COM object is just a pointer to a vtable that
contains a QueryInterface method that returns "this" for IUnknown and
fails in every other case, an AddRef method that returns 2 and does
nothing, and a Release method that returns 1 and does nothing else.


> +    /* pUnk==NULL, pDP!=NULL */
> +    hr = DirectPlayCreate( NULL, &pDP, NULL );
> +    _okHR( DPERR_INVALIDPARAMS, hr );
> +    hr = DirectPlayCreate( (LPGUID) &_GUID_NULL, &pDP, NULL );
> +    _okHR( DP_OK, hr );
> +    hr = DirectPlayCreate( (LPGUID) &DPSPGUID_TCPIP, &pDP, NULL );
> +    todo_wine _okHR( DP_OK, hr );

Here you are leaking DirectPlay objects. If DirectPlayCreate worked, you
have to release the object before you overwrite pDP (by passing it to
DirectPlayCreate again) using IDirectPlay_Release(pDP).

Regards,
  Michael Karcher




More information about the wine-devel mailing list