Am Donnerstag, den 10.07.2008, 22:04 +0300 schrieb Ismael Barros:
- 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.
[...]
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.
For me, the create-heap-and-throw-away approach is OK, but as it is not the way it is usually handled in wine, I would like to see a comment. Perhaps AJ prefers cyclic buffers or throw-away heaps. Or perhaps he doesn't care how it is done in test cases.
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);
I don't think this line is good style, but I still like it more than your program, as this will definitely crash if the pointer is derefenced, while your variant makes the function derefencing the pointer accessing uninitialized data.
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),
Thats bad. Every COM object must return a valid interface pointer for IID_IUnknown. And even worse: The IID_IUnknown pointer *has* to be stable. So if you do the following (pretend that CreateFooBar(REFIID, DWORD, IUnknown*) is a factory function:
IFooBar *fb1, *fb2; IUnknown *unk1, *unk2; fb1 = CreateFooBar(IID_IFooBar, 0, NULL); IFooBar_QueryInterface(fb1, IID_IUnknown, &unk1); IFooBar_QueryInterface(fb1, IID_IFooBar, (IUnknown*)&fb2); IFooBar_Release(fb1); IFooBar_QueryInterface(fb2, IID_IUnknown, &unk2); IFooBar_Release(fb2); ok(unk1 == unk2, "COM violation: IUnknown interface must be stable!"); IUnknown_Release(unk1); IUnknown_Release(unk2);
The COM specification guarantees the test to succeed, independent of what type IFooBar has! DirectPlay currently has no permanent COM object, but creates a new object on the fly in QueryInterface; this is needed to support the different vtables.
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?
If they were permanent, it would be OK. All COM interfaces are derived from IUnknown, and can thus be used as IUnknown. The only catch is the object lifetime uniqueness of the IUnknown interface.
This seems to be done a lot in other dlls)
For ddraw for example, it is correct. The IDirectDrawImpl object is live until all references to it (including the IUnknown one) are gone.
Regards, Michael Karcher