Re: dpnet: Improve Error checking in Get/Set SP

[Stefan Dösinger]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi,
> +        hr = IDirectPlay8Address_GetSP(localaddr, &guidsp);
> +        ok(hr == DPNERR_DOESNOTEXIST, "got 0x%08x\n", hr);
> For the sake of completion I'd test setting GUID_NULL, IID_IUnknown and something nonsensical like IID_IDirectDrawGammaControl (or maybe something you have access to without additional headers like IID_IDirectPlay8Address).

Some style nitpicks follow. If you end up becoming the de-facto maintainer of dplay it's up to you how important they are:
> +    sprintf(buf,
> +        "{%08x-%04x-%04x-%02x%02x-%02x%02x%02x%02x%02x%02x}",
> +        guid->Data1, guid->Data2, guid->Data3,
> +        guid->Data4[0], guid->Data4[1], guid->Data4[2], guid->Data4[3],
> +        guid->Data4[4], guid->Data4[5], guid->Data4[6], guid->Data4[7]);
> In the d3d code we use 8 space indentations. You may want to follow that.

Also, there's debugstr_guid in wine/debug.h. It seems you're aware of that and used your own functions because wine/debug.h doesn't work in the tests. Is this correct?
> +  if(!pguidSP)
> +    return DPNERR_INVALIDPOINTER;
> ...
> +    if( SUCCEEDED(hr)  )
> +    {
> ...
> +    if (hr != S_OK)
> The whitespaces around the () are inconsistent.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJS1kqJAAoJEN0/YqbEcdMwfngP/RUrjrAH/vDvQH+jtBSOQwd8
OymWNEWmt4jqzt3febi5Mo15oDGSuuqIjcyoRwpOTPrlmwGYmeAjm/z5JxAcH+o1
XFhZF8zL85u0sAlW8A3bAEuPB/a0nP2/K8y523mt+uFYz/4cOwv11sR/fAJjCgR5
SkkWI29t/JXBkQmPhQHArdyi1CgRTQ+MSNEwzM3B+nudn8zhjV3VH+WYDyLkl8D6
GS3GBJqAAka0Iqo6KdVC4VTdxSVUJVivfAZSlSTZVHcYAlC49k6F5uYtTLd+MQRZ
bUlnHSkJtveC1lj9FiIliJexO1M9zLr+lY5/RtD9ntbe4K5i+tgEpjExg6uSNbmD
cw6IjE+qcrQsp2x9NBED7j1OetUmjeUgwUQcWo56FoVQBp9N7ynSLF2HOPQ7rf8Q
SHroMxtOzwPHxmyXNFDCbDYwnVviq17KqSOXcD46VvY99NbFEmm5nyPBNuLUzPQb
JkkakS4i1+eUigD3QnY5eHE5NPWJBq4/WeEF9TYtBUjg30Q0pU5CmDf888fyFPj2
UYsheWKR5LcUIZ3oXFPrzcaiS+1hLAfEiKCH8VXsNCPMV4S6KDS2hIhHJQh3mWEj
z//uLe23Ll2bNrZOOfzwWBGfAmLNB+TfW+Q2phXlTTH5lgITxU4N0KR/pNpCkqz8
KrWsuFvS8iZSfT3G+fSw
=XG/n
-----END PGP SIGNATURE-----

dpnet: Improve Error checking in Get/Set SP

[Michael Stefaniuc]

On 01/15/2014 09:44 AM, Stefan Dösinger wrote:
> Hi,
> > +        hr = IDirectPlay8Address_GetSP(localaddr, &guidsp); +
> > ok(hr == DPNERR_DOESNOTEXIST, "got 0x%08x\n", hr);
> > For the sake of completion I'd test setting GUID_NULL, IID_IUnknown
> > and something nonsensical like IID_IDirectDrawGammaControl (or
> > maybe something you have access to without additional headers like
> > IID_IDirectPlay8Address).
> > I nowadays use IID_IClassFactory for the "invalid" IID test. Just to
> > avoid having to include an other header.
> Some style nitpicks follow. If you end up becoming the de-facto
> maintainer of dplay it's up to you how important they are:
> > +    sprintf(buf, +
> > "{%08x-%04x-%04x-%02x%02x-%02x%02x%02x%02x%02x%02x}", +
> > guid->Data1, guid->Data2, guid->Data3, +        guid->Data4[0],
> > guid->Data4[1], guid->Data4[2], guid->Data4[3], +
> > guid->Data4[4], guid->Data4[5], guid->Data4[6], guid->Data4[7]);
> > In the d3d code we use 8 space indentations. You may want to follow
> > that.
> > It is double indentation for continuation lines. Or align to the
> > opening "("; don't know what dplay and related prefer. Probably a mix
> > of both.
> Also, there's debugstr_guid in wine/debug.h. It seems you're aware
> of that and used your own functions because wine/debug.h doesn't
> work in the tests. Is this correct?
> I keep running into the need of debugstr_guid() in tests so I plan to
> add it to include/wine/test.h and remove the duplicates. But I'm busy
> atm with dmusic and don't want to stop.

bye
	michael