Re: Should I give up? => Please do not give up

[Detlef Riekenberg]

Hi Damjan:
> My work on the still image system for wine has
> while wine incorrectly tries to open
> HKEY_LOCAL_MACHINE\System\CurrentControlSet\
>   Class\xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx
> ie. no curly braced around the guid.
> And in addition it incorrectly returns FALSE for a
> return type of HKEY (instead of INVALID_HANDLE_VALUE).

It's always a good Idea, to fix only one Bug at once.
> I've submitted a test and patch to wine-patches 7
> times now, and received very little feedback, which
> I've always followed.

Please do not give up.

When a Patch goes in, depends not only on the actual
Patch-Quality / Size and what Patches came from the
Creator before, but also the actual amount of Patches
in the Pipeline and the Area, the Patch is for.
(I have similar Problems for my Patches about printing,
but at the moment, there was/is: Google SoC, 
Crossover-Office Mac beta, AJ on Holiday, wineconf)
Also comments from other Developer about Patch-review
can affect the time, a Patch needs to go in the tree.
When there are some People working for the same Area
(d3d as example), patches are commented / reviewed by
others from the group (most times on #winehackers).

Reviewing Patches for an unusual Area is boring
and Applications do not use Device-installation
very often, but I took a look.

You mixed TAB and SPACE. (Common used for idention is 4 SPACE)
As already mentioned, testing the ANSI-version is prefered
and simplifies the test.
UNICODE - Tests do not test the String - convertations.
> @@ -1453,21 +1454,30 @@
>      if (UuidToStringW((UUID*)ClassGuid, &lpGuidString) != RPC_S_OK)
> - lpBracedGuidString = (LPWSTR) HeapAlloc(GetProcessHeap(), 0,
> -
>    (1 + strlenW(lpGuidString) + 1 + 1) * sizeof(WCHAR));
> - lpBracedGuidString[0] = (WCHAR) '{';
> - memcpy(&lpBracedGuidString[1], lpGuidString,
> -
>    strlenW(lpGuidString) * sizeof(WCHAR));
> - lpBracedGuidString[1 + strlenW(lpGuidString)] = (WCHAR) '}';
> - lpBracedGuidString[1 + strlenW(lpGuidString) + 1] = (WCHAR) 0;
> - RpcStringFreeW(&lpGuidString);

After looking in rpcrt4_main.c, RpcStringFreeW looks ok.
> -
>   RpcStringFreeW(&lpGuidString);
> -
>    RpcStringFreeW(&lpBracedGuidString);

You should use HeapFree here, because you used HeapAlloc above.
This makes you independant from changes inside rpcrt4.
>     RegCloseKey(hClassesKey);
> -
>   return FALSE;
> -
>   return INVALID_HANDLE_VALUE;
> }

With one fix as a time, the patch get's smaller and can be
reviewed more easy.
> - RpcStringFreeW(&lpGuidString);
> - RpcStringFreeW(&lpBracedGuidString);

Same as above: HeaAlloc -> HeapFree
> +static void test_SetupDiOpenClassRegKeyExW()

Parameter not used requires:  (VOID)
> - if (rpcStatus != RPC_S_OK)
> - {
> -
>    ok(FALSE, "failed to generate test guid, error %ld",
>
> rpcStatus);

Already mentioned in other Mails.
> - /* Check return value for non-existant key */
> - hkey = pSetupDiOpenClassRegKeyExW(&guid, KEY_ALL_ACCESS,
> -
>    DIOCR_INSTALLER, NULL, NULL);
> - ok(hkey == INVALID_HANDLE_VALUE,
> -
>    "invalid return value %p from SetupDiOpenClassRegKeyExW "
> -
>    "for non-existant key, expected %p\n", hkey,
>
> INVALID_HANDLE_VALUE);

The Message is very long. I suggest not more as:
ok(hkey == INVALID_HANDLE_VALUE, 
    "returned %p (expected INVALID_HANDLE_VALUE)\n", hkey)
"Test failed:" is prefixed by ok()
> - bracedGuid = HeapAlloc(GetProcessHeap(), 0,
> -
>    (lstrlenW(unbracedGuid) + 3) * sizeof(WCHAR));
> - if (bracedGuid != NULL)
> - {

When the allocation failed, return from the test here.
This saves you a lot of idention.

But since you are testing for a non-existend value,
do you really need an unique Value here?
Can this be tested with "all '0'" or "all 'F'" ?
In this case, all value can be const string.
The test will be a lot short and even rpcrt4.dll
is not needed.