Re: [2/2] d3dx9_36/tests: Add tests for D3DXCreatePolygon. (try 3)

[Matteo Bruni]

2014-11-04 19:57 GMT+01:00 Sebastian Lackner :
> Based on a patch by David Adam.
>
> dlls/d3dx9_36/tests/mesh.c | 163 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 163 insertions(+)

Hi Sebastian,

first of all, let me say I'm okay with these patches going in as they are.

IMHO there is still room for improvement though:

+static void test_polygon(IDirect3DDevice9 *device, float length,
unsigned int sides)
+{
+    HRESULT hr;
+    ID3DXMesh *polygon;
+    struct mesh mesh;
+    char name[256];

No need to use such a big array there.

+    hr = D3DXCreatePolygon(device, length, sides, &polygon, NULL);
+    ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK)\n", hr);
+    if (hr != D3D_OK)
+    {
+        skip("Couldn't create box\n");

Copy-paste typo. FWIW, we usually prefer a '.' at the end of trace messages.

+    adjacency = NULL;
+    hr = D3DXCreatePolygon(device, 3.0f, 11, &polygon, &adjacency);
+    ok(hr == D3D_OK, "Expected D3D_OK, received %#x\n", hr);
+
+    if (FAILED(hr))
+    {
+        skip("D3DXCreatePolygon failed\n");
+        goto end;
+    }

That D3DXCreatePolygon() call is not expected to ever fail, so no need
for the "if".

+    if (adjacency)
+        ID3DXBuffer_Release(adjacency);

If you remove the previous "if" you won't need the "end" label and
this one becomes always true. Then I'd put this Release() right after
the loop checking the adjacency.

Again, I don't care about those details, this is still good enough for
me. It's mostly about any future patch you'd want to send in.

[2/2] d3dx9_36/tests: Add tests for D3DXCreatePolygon. (try 3)

[Sebastian Lackner]

I am also fine with fixing these issues right away, especially because of the ugly typo - the error check for D3DXCreatePolygon() was copied from existing tests, I assumed there is a good reason why D3DXCreate*() could fail on some machines/testbots.
Would you mind taking another look please? Thanks for your feedback!

Regards,
Sebastian

On 05.11.2014 18:03, Matteo Bruni wrote:
> 2014-11-04 19:57 GMT+01:00 Sebastian Lackner :
> > Based on a patch by David Adam.
> >
> > dlls/d3dx9_36/tests/mesh.c | 163 +++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 163 insertions(+)
>
> Hi Sebastian,
>
> first of all, let me say I'm okay with these patches going in as they are.
>
> IMHO there is still room for improvement though:
>
> +static void test_polygon(IDirect3DDevice9 *device, float length,
> unsigned int sides)
> +{
> - HRESULT hr;
> - ID3DXMesh *polygon;
> - struct mesh mesh;
> - char name[256];
>
> No need to use such a big array there.
> - hr = D3DXCreatePolygon(device, length, sides, &polygon, NULL);
> - ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK)\n", hr);
> - if (hr != D3D_OK)
> - {
> -
>    skip("Couldn't create box\n");
>
> Copy-paste typo. FWIW, we usually prefer a '.' at the end of trace messages.
> - adjacency = NULL;
> - hr = D3DXCreatePolygon(device, 3.0f, 11, &polygon, &adjacency);
> - ok(hr == D3D_OK, "Expected D3D_OK, received %#x\n", hr);
> -
> - if (FAILED(hr))
> - {
> -
>    skip("D3DXCreatePolygon failed\n");
> -
>    goto end;
> - }
>
> That D3DXCreatePolygon() call is not expected to ever fail, so no need
> for the "if".
> - if (adjacency)
> -
>    ID3DXBuffer_Release(adjacency);
>
> If you remove the previous "if" you won't need the "end" label and
> this one becomes always true. Then I'd put this Release() right after
> the loop checking the adjacency.
>
> Again, I don't care about those details, this is still good enough for
> me. It's mostly about any future patch you'd want to send in.

[2/2] d3dx9_36/tests: Add tests for D3DXCreatePolygon. (try 3)

[Matteo Bruni]

2014-11-05 18:30 GMT+01:00 Sebastian Lackner :
> I am also fine with fixing these issues right away, especially because of the ugly typo - the error check for D3DXCreatePolygon() was copied from existing tests, I assumed there is a good reason why D3DXCreate*() could fail on some machines/testbots.

I think that kind of check only makes sense if you write the test
before the implementation, in that case you need something like that
to avoid the crash when running the test with builtin d3dx9. I haven't
looked but probably the patch implementing the function removed the
todo_wine from the test but forgot to remove the if.
At least, I can't think of any other reason for that to fail in the test.
> Would you mind taking another look please? Thanks for your feedback!

Sure, going to take a look.
> Regards,
> Sebastian
>
> On 05.11.2014 18:03, Matteo Bruni wrote:
> > 2014-11-04 19:57 GMT+01:00 Sebastian Lackner :
> > > Based on a patch by David Adam.
> > >
> > > dlls/d3dx9_36/tests/mesh.c | 163 +++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 163 insertions(+)
> >
> > Hi Sebastian,
> >
> > first of all, let me say I'm okay with these patches going in as they are.
> >
> > IMHO there is still room for improvement though:
> >
> > +static void test_polygon(IDirect3DDevice9 *device, float length,
> > unsigned int sides)
> > +{
> > - HRESULT hr;
> > - ID3DXMesh *polygon;
> > - struct mesh mesh;
> > - char name[256];
> >
> > No need to use such a big array there.
> > - hr = D3DXCreatePolygon(device, length, sides, &polygon, NULL);
> > - ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK)\n", hr);
> > - if (hr != D3D_OK)
> > - {
> > -
> >    skip("Couldn't create box\n");
> >
> > Copy-paste typo. FWIW, we usually prefer a '.' at the end of trace messages.
> > - adjacency = NULL;
> > - hr = D3DXCreatePolygon(device, 3.0f, 11, &polygon, &adjacency);
> > - ok(hr == D3D_OK, "Expected D3D_OK, received %#x\n", hr);
> > -
> > - if (FAILED(hr))
> > - {
> > -
> >    skip("D3DXCreatePolygon failed\n");
> > -
> >    goto end;
> > - }
> >
> > That D3DXCreatePolygon() call is not expected to ever fail, so no need
> > for the "if".
> > - if (adjacency)
> > -
> >    ID3DXBuffer_Release(adjacency);
> >
> > If you remove the previous "if" you won't need the "end" label and
> > this one becomes always true. Then I'd put this Release() right after
> > the loop checking the adjacency.
> >
> > Again, I don't care about those details, this is still good enough for
> > me. It's mostly about any future patch you'd want to send in.