Re: [2/2] ddraw: add dsurface dimension tests, try 11

[Henri Verbeet]

On 30 June 2014 17:42, Patrick Rudolph  wrote:
> - const static struct

"static const"
> -
>            {16}, {0x0000F800}, {0x000007E0}, {0x0000001F}, {0x00000000}

Please use lowercase hex constants.
> - static struct
> - {
> -
>    DWORD dwFlags;
> -
>    DWORD dwCaps;
> -
>    DWORD dwWidth;
> -
>    DWORD dwHeight;
> -
>    HRESULT hr;
> - }

Please drop the dw prefixes and capitals from the field names here.
> -
>    /* 6: Test maximum surface height */
> -
>    {DDSD_CAPS | DDSD_WIDTH | DDSD_HEIGHT, DDSCAPS_OFFSCREENPLAIN, 1, 0x10000, DDERR_INVALIDPARAMS},
> -
>    /* 7: Test maximum surface width */
> -
>    {DDSD_CAPS | DDSD_WIDTH | DDSD_HEIGHT, DDSCAPS_OFFSCREENPLAIN, 0x10000, 1, DDERR_INVALIDPARAMS},

It would perhaps also be useful to verify that e.g. a 1 by 0xffff
surface actually is allowed.
> -
>    /* 8: Test OUTOFVIDEOMEMORY */
> -
>    {DDSD_CAPS | DDSD_WIDTH | DDSD_HEIGHT, DDSCAPS_OFFSCREENPLAIN | DDSCAPS_VIDEOMEMORY,
> -
>        0xffff, 0xffff, DDERR_OUTOFVIDEOMEMORY},

Double indentation here, please.
> - ddcaps.dwSize = sizeof(DDCAPS);

ddcaps.dwSize = sizeof(ddcaps);
> - /* expect an error if the hardware doesn't support videomemory */
> - if (!(ddcaps.ddsCaps.dwCaps & DDSCAPS_VIDEOMEMORY))
> -
>    tests[8].hr = DDERR_NODIRECTDRAWHW;

You can just handle that when testing the expected result. (And then
"tests" can just be const.) E.g.:

hr = IDirectDraw_CreateSurface(...);
if (tests[i].caps & DDSCAPS_VIDEOMEMORY && !(ddcaps.ddsCaps.dwCaps &
DDSCAPS_VIDEOMEMORY))
    ok(hr == DDERR_NODIRECTDRAWHW, ...);
else
    ok(hr == tests[i].hr, ...);
> -
>        surfacedesc.dwFlags = tests[i].dwFlags | DDSD_PIXELFORMAT;

It seems arbitrary to put DDSD_CAPS in the table, but not
DDSD_PIXELFORMAT. IMO either both should be added here, or both should
be in the table.
> -
>        hr = IDirectDraw_CreateSurface(ddraw, &surfacedesc, &surface, NULL);
> -
>        ok(hr == tests[i].hr, "Pixelformat %s, test %d returned %08x\n", formats[j].name, i, hr);
> -
> -
>        if (surface)
> -
>        {
> -
>            IDirectDrawSurface_Release(surface);
> -
>            surface = NULL;
> -
>        }

hr = IDirectDraw_CreateSurface(...);
...
if (FAILED(hr))
    continue;
IDirectDrawSurface_Release(surface);
> - ok(SUCCEEDED(hr), "CreateSurface returned %08x\n", hr);
> - if (surface)

The if is redundant here, since you're claiming that creating the
surface should always succeed a line earlier.
> -
>    surface = NULL;

This doesn't really do anything, since "surface" is never used afterwards.