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