[dplayx 02/29] Tests for DirectPlayCreate

[Ismael Barros]

Thanks for the review :)

On 7/9/08, Michael Karcher <wine at mkarcher.dialup.fu-berlin.de> wrote:
> 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 );

Depending on the situation I write ok( DP_OK == hr ) instead of ok( hr
== DP_OK, ... ), specially if instead of "hr" there is a DirectPlay
call or some complex expression, in order to improve readability.
Regarding this second case, I decided to use the (expected, result)
order, but it's true that sometimes it can be confusing.

> Why do you start the macro name with an underscore?

Because _okHR compares HRESULT codes, but there's also _ok to compare
integers, or _okFlags to compare flags (the main difference being how
to print info about the expected and actual values), or _okStr to
compare strings.

> And last but not
> least: As I see it, a static inline would do the job too, and is always
> preferrable to macros.

The main reason of using ditry macros instead of functions was to
preserve the __LINE__ number, otherwise the debug information provided
is pretty much useless.

>> +    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) );

That's what I used in the begining, but I've got some HeapAllocs that
I don't free. This is intended, as freeing that memory would make the
code unnecessarily complex, I can't use static local variables (I use
the result of the function inside the same ok() or trace()), and
loosing memory is always undesirable but not critical in a test case.
The main problem was to get yelled when valgrind complains about the
memory leak, and Kai Blin advised me to use my own heap to avoid
valgrind complains.
Anyway is this is too dirty I'll have no problem in doing it "correctly".

> 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.

I just wrote is to check the return value in case of pUnk != NULL, but
there would be no problem in either removing that test (it's probably
unneeded) or doing it the right way.

>
>> +    /* 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).

Roger

> Regards,
>   Michael Karcher
>
>