[dplayx 02/29] Tests for DirectPlayCreate

[Ismael Barros]

>> > 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.
> Yes, you are right of course. I didn't think of __LINE__ (also James
> noted the same thing). Now you get to the point where a precedence case
> for identifiers with an underscore are used: shell32/tests/shlexec.c.
> They have inline functions, called _okFooBar which take file and line as
> parameter, and have a macro okFooBar which calls the _okFooBar method,
> so the test case writer does not get to see the identifier starting with
> an underscore. Thats how I feel about them: They are internal
> identifiers, telling you "if you use this directly, you better know very
> well what you are doing; most probably there is a safer or easier way to
> accomplish the goal". In this case, the _okFooBar function is meant just
> for the easier okFooBar macro.

Sounds fine, I'll change it.

>> >> +    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.
> OK. That's a point. I didn't expect it, though. I would like a comment
> in there along
>   /* Create an extra heap to throw away garbage after tests */

No problem.

>> 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.
> You can use static variables, if you go along the lines of of
> get_temp_buffer in libs/wine/debug.c, where a static local array is
> cyclical indexed. This might not be a good idea, though, depending on
> how long the result has to stay alive.

Hm, yeah, I thought about a cyclic buffer but wanted to keep the code simple. However this heap thing is creating some skepticism, so I'll go that way.

>> > 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.
> A test case is always good, if it does not take too much execution time.
> These tests with NULL pointers seemed strange to me when I first
> encountered, as sometimes NULL pointers are passed into functions where
> they are not useful or even forbidden by the documented contract. But as
> wine is all about fulfilling the same contract as the windows
> implementation which might deviate from MSDN, the test case serves as
> documentation of Windows behavior, which is good.
>
> But in my opinion, testing of the reaction to contract violation should
> not go into passing somehow valid-looking pointers, that do not point to
> anything sensible. And the main point was, that I really don't get why
> it must be a heap pointer. The an LPDWORD on the stack would do as well
> as allocating sizeof(LPDWORD) area.

I've been checking how this is done in other tests, and it's particularly interesting how they do it in ddraw/tests/refcount.c:81:
  hr = IDirectDraw7_CreatePalette(DDraw7, DDPCAPS_ALLOW256 | DDPCAPS_8BIT, Table, &palette, (void *) 0xdeadbeef);
This is the only example in the current code. I could either do it that way (not correct, but simple), or use some IUnknown interface. I tried to get the IUnknown of the classes I have access to, DirectPlay or DirectPlayLobby, but the current implementation is not able to return an interface for IID_IUnknown (it just fails with E_NOINTERFACE), so I would have to fix that too (and I'm not very sure about how to do it, without breaking anything. Would returning any of the existing interfaces that have CreateInstance, AddRef and Release, be correct? This seems to be done a lot in other dlls)