Re: [PATCH 2/5] ddraw/tests: Port the depth blit test to ddraw7 (try 2)

[Henri Verbeet]

On 22 January 2012 16:32, Stefan Dösinger  wrote:
> +static void get_depth_stencil(IDirect3DDevice7 *device, IDirectDrawSurface7 **ds)
> +{
> - IDirectDrawSurface7 *rt;
> - DDSCAPS2 caps = {DDSCAPS_ZBUFFER, 0, 0, 0};
> - HRESULT hr;
> -
> - hr = IDirect3DDevice7_GetRenderTarget(device, &rt);
> - ok(SUCCEEDED(hr), "Failed to get the render target, hr %#x.\n", hr);
> - hr = IDirectDrawSurface7_GetAttachedSurface(rt, &caps, ds);
> - ok(SUCCEEDED(hr) || hr == DDERR_NOTFOUND, "Failed to get the z buffer, hr %#x.\n", hr);
> - IDirectDrawSurface7_Release(rt);
>
> +}

This should probably just return the surface.
> - hr = IDirect3D7_QueryInterface(d3d, &IID_IDirectDraw7, (void **) &ddraw);

Extra space.
> - ok(SUCCEEDED(hr), "Failed to set the z cuntion, hr %#x.\n", hr);

Typo.
> - memset(&fx, 0, sizeof(fx));
> - fx.dwSize = sizeof(fx);
> - fx.dwFillDepth = 0;

That probably needs an U5(). I tend to forget half of them as well though.

[PATCH 2/5] ddraw/tests: Port the depth blit test to ddraw7 (try 2)

[Stefan Dösinger]

Am Montag, 23. Januar 2012, 13:47:47 schrieb Henri Verbeet:
> > - memset(&fx, 0, sizeof(fx));
> > - fx.dwSize = sizeof(fx);
> > - fx.dwFillDepth = 0;
>
> That probably needs an U5(). I tend to forget half of them as well though.

The line is actually redundant. I noticed that when I looked over the other 
tests before sending them, but forgot to remove it from this version. Although 
you could argue that it makes the code more readable - do you have any 
preference before I resend?

@Dan: With things like this one the buildbot could be really helpful. For the 
nameless union issue we don't even need the tests, so we could put the 
buildbot to use before we have all the flakyness bugs solved.

[PATCH 2/5] ddraw/tests: Port the depth blit test to ddraw7 (try 2)

[Francois Gouget]

On Mon, 23 Jan 2012, Stefan Dösinger wrote:
> Am Montag, 23. Januar 2012, 13:47:47 schrieb Henri Verbeet:
> > > - memset(&fx, 0, sizeof(fx));
> > > - fx.dwSize = sizeof(fx);
> > > - fx.dwFillDepth = 0;
> >
> > That probably needs an U5(). I tend to forget half of them as well though.
>
> The line is actually redundant. I noticed that when I looked over the other 
> tests before sending them, but forgot to remove it from this version. Although 
> you could argue that it makes the code more readable - do you have any 
> preference before I resend?
>
> @Dan: With things like this one the buildbot could be really helpful. For the 
> nameless union issue we don't even need the tests, so we could put the 
> buildbot to use before we have all the flakyness bugs solved.

Dan actually set up a build slave to check for nameless union issues. 
However I'm not sure all developers would be happy to have to resubmit 
their patches due to such an issue. An option may be to put that build 
slave in a 'pedantic' category that sends a notification to the user but 
is not grounds for rejecting the patch.

In the meantime I don't mind fixing these issues. I automatically get a 
report for these on Saturday morning and will usually send the patch 
over the week-end.