Hi The library I was working on last weeks - gameux.dll - is now partially complete. IGameExplorer and IGameExplorer2 interfaces are working and stores data in Windows-compatible format (attached tests confirm it). Last two interfaces - IGameStatistics and IGameStatisticsMgr has initial work done, but, as their task is only read and save data files, it's not much work to complete them.
Please review my code and give some feedback about it. Thanks in advance.
Overall, I don't feel this series is "ready" yet, or that it will be ready if you address all of my comments. I'm intentionally keeping quiet about some things because I think it will be better for you to address them when the code freeze is over and you can send things to wine-patches. This is mostly because I'm not very good at predicting what will be accepted, what won't, and (most importantly) why.
+IMPORTS = ole32 user32 kernel32 advapi32 oleaut32 msvcrt shell32
The msvcrt import looks suspicious to me, but I can't say how you should avoid using the wtof function later.
+ skip("IGameExplorer cannot be created\n");
I believe that winetest.exe will not start the test on Windows systems that are missing gameux.dll, so you shouldn't have to worry about this as long as all native versions of gameux.dll contain the class.
I can't comment on the kernel32 and winnt.h patches, or the use of msxml.
+ if(GDFData->sName) CoTaskMemFree(GDFData->sName);
You don't have to check for NULL here. CoTaskMemFree can free NULL (as can most other free functions).
(I've only read patches 1-12 so far. My attention started to flag at patch 13. Now I understand why Henri sends only 5 patches at a time. I will return to this later.)
2010/7/9 Mariusz Pluciński vshader@gmail.com:
Hi The library I was working on last weeks - gameux.dll - is now partially complete. IGameExplorer and IGameExplorer2 interfaces are working and stores data in Windows-compatible format (attached tests confirm it). Last two interfaces - IGameStatistics and IGameStatisticsMgr has initial work done, but, as their task is only read and save data files, it's not much work to complete them.
Please review my code and give some feedback about it. Thanks in advance.
+ if( CompareStringW(LOCALE_INVARIANT, 0, bstrElementName, nElementNameLen, sNodeExtendedProperties, lstrlenW(sNodeExtendedProperties)) == CSTR_EQUAL)
I think you can get away with an lstrcmpW here. Yes, technically it can behave differently depending on locale, but you're doing a case-sensitive compare with a string with only ascii characters. Also, everyone else uses lstrcmpW.
+ GDFData->sName = (LPWSTR)CoTaskMemAlloc(nValueLen*sizeof(WCHAR));
It's not necessary to cast a void pointer.
+ GDFData->fPerformanceRatingMinimum = _wtof((LPCWSTR)bstrValue);
Maybe you could avoid converting this to a float, or perhaps convert to ANSI first? Then you wouldn't have to import msvcrt.
+HRESULT _ParseGameDefinitionGenres(IXMLDOMElement* pXMLGameDefinitionElement, + struct GAME_DEFINITION_FILE_DATA* GDFData) +{ + FIXME("stub\n"); + return S_OK; }
I think you should avoid internal stub functions. Instead, you should initially leave out the code that uses your internal function, then add a partially implemented implemented internal function and code that uses it in the same patch.
+ LPWSTR values[5];
Can there be more than 5 genres?
Perhaps you should check the length of the IXMLDOMNodeList and allocate your array first based on that length?
Same applies to the other node types with unknown sizes.
+ else if( CompareStringW(LOCALE_INVARIANT, 0, bstrElementName, nElementNameLen, sGameTasks, lstrlenW(sGameTasks)) == CSTR_EQUAL ) + {
The level of indentation that ensues after this makes it unreadable. Perhaps you could add a few extra helper functions for this? (And note that you don't have to put each helper function implementation in its own patch.)
+ ZeroMemory((*pTask)->sName, nValueLen*sizeof(WCHAR)); + lstrcpynW((*pTask)->sName, bstrValue, nValueLen);
No need to zero a string you're about to copy over.
In patch 22, you leak a registry key if there's an error in one of the writes.
Here's the pattern I generally use when I have to make a large series of calls, each of which may return an error code.
If I'm calling a function that creates something that I need to free, I do it like this:
hr = CreateThing(&thing); if (SUCCEEDED(hr)) { [Code that uses thing]
FreeThing(thing); }
or
thing = CreateThing(); if (thing) { [Code that uses thing]
FreeThing(thing); } else hr = E_OUTOFMEMORY;
Now here's the cool part. If I need to make a series of calls, I can chain them like this:
hr = DoSomething();
if (SUCCEEDED(hr)) hr = DoSomethingElse();
if (SUCCEEDED(hr)) hr = DoAnotherThing();
This keeps the level of indentation down and avoids the use of goto. Although it looks like there are unnecessary checks when the first call fails, the compiler knows that if the first if check fails, so will the second. So this leads to efficient code.
+ * _delete_GAME_DEFINITION_FILE_DATA_TASK is private internal + * function used ONLY by normal _delete_GAME_DEFINITION_FILE_DATA, + * please do not use it manually */ void _delete_GAME_DEFINITION_FILE_DATA_TASK(struct GAME_DEFINITION_FILE_DATA_TASK **pTask)
Sounds like a good reason to make it static. (In fact, all internal functions meant to be used only in one C file should be static.)
OK, stopping for the day at patch 26. (I really hate that we had the code freeze during summer of code, leading to this buildup of unsent code.)
By the way, I think you're doing a great job. Your overall design and approach look absolutely right to me. That's the hard part. The only problems I can see with your code so far are in implementation details and in exactly how the code is divided into patches.
Hello First, thank you very much for checking my code. That's good to know my design is valid and I can continue creating code in this way. Of course, I know that my code will probably be rejected on the beginning, but it's still better for me when someone reviews my code, than working completely without checking all the time.
I believe that winetest.exe will not start the test on Windows systems that are missing gameux.dll, so you shouldn't have to worry about this as long as all native versions of gameux.dll contain the class.
I didn't know about that. Thanks.
I think you can get away with an lstrcmpW here. Yes, technically it can behave differently depending on locale, but you're doing a case-sensitive compare with a string with only ascii characters. Also, everyone else uses lstrcmpW.
You're right, I'll replace it.
+IMPORTS = ole32 user32 kernel32 advapi32 oleaut32 msvcrt shell32
The msvcrt import looks suspicious to me, but I can't say how you should avoid using the wtof function later.
- GDFData->fPerformanceRatingMinimum =
_wtof((LPCWSTR)bstrValue);
Maybe you could avoid converting this to a float, or perhaps convert to ANSI first? Then you wouldn't have to import msvcrt.
Why should I avoid using msvcrt, when e.g. Microsoft's implementation of gameux.dll uses it?
I think you should avoid internal stub functions. Instead, you should initially leave out the code that uses your internal function, then add a partially implemented implemented internal function and code that uses it in the same patch.
If not code freeze, I would know about it after sending first patch with internal stub. Now I need to fix a little more code.
- LPWSTR values[5];
Can there be more than 5 genres?
Perhaps you should check the length of the IXMLDOMNodeList and allocate your array first based on that length?
Same applies to the other node types with unknown sizes.
MSDN says that there can be maximum 5 genres. If there are more than 5 in GDF, I simply skip them. This is why I used array with fixed size.
hr = DoSomething();
if (SUCCEEDED(hr)) hr = DoSomethingElse();
if (SUCCEEDED(hr)) hr = DoAnotherThing();
Thanks for great strategy. I also dislike when there's lot of indentations, but didn't know how to solve it without goto (which in my opinion is even more unreadable).
(In fact, all internal functions meant to be used only in one C file should be static.)
I must admit, that Wine is first code in my life where I saw wide usage of static functions in C. This is probably why I often forget about using them.
Again, thanks very much. I hope that code freeze will really finish in this week, and your advices will increase chances for my code to be accepted.
Whew, last one.
_FindGameByGDFBinaryPathInScope doesn't free lpData.
It also never returns S_FALSE, but I see you fixed that later. You might want to add a test for GetGameStatistics in the case where a game is installed for all users or not installed at all. That should exercise the S_FALSE cases, if I understand this.
+ bstrGDFBinaryPath = SysAllocString(binaryGDFPath); + if(!bstrGDFBinaryPath) + hr = E_OUTOFMEMORY; + else + { + hr = IGameExplorer_VerifyAccess(IGameExplorer_from_impl(This), bstrGDFBinaryPath, pHasAccess); + SysFreeString(bstrGDFBinaryPath); + }
Given that VerifyAccess requires a BSTR and CheckAccess does not, it might make more sense to implement VerifyAccess based on CheckAccess.
As for why you shouldn't use msvcrt, well, I don't have a good answer for that. I just know seeing it in the makefile doesn't give me a good feeling, and no one else is using it. I asked in #winehackers and was told it can interfere with linking to the native libc.