Am Mittwoch, den 09.07.2008, 20:15 +0300 schrieb Ismael Barros:
Thanks for the review :)
No problem.
On 7/9/08, Michael Karcher wine@mkarcher.dialup.fu-berlin.de wrote:
I dislike the order of the parameters and the name. You would write
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.
OK, taking back the criticism on parameter order. Passing the actual value last is common practice in the user32 test, so your variant has a precedence case in wine. Yet I couldn't find these kind of macros starting with an underscore. More on that point below.
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.
- 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 */
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.
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.
Regards, Michael Karcher