[RFC] gameux.dll partial implementation

[Vincent Povirk]

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