Re: [PATCH 2/2] d3drm/tests: Add QueryInterface tests for IDirect3DRMTexture*.

[Henri Verbeet]

On 18 January 2016 at 19:57, Aaryaman Vasishta
 wrote:
> - char *file = NULL;

Why do you initialize this to NULL?
> - BOOL check;
> -
> -
> - hr = Direct3DRMCreate(&d3drm1);

The extra empty line is weird. Is that intentional?
> - file = create_bitmap(1, 1, TRUE);
> - hr = IDirect3DRM_LoadTexture(d3drm1, file, &texture1);

I think "file" is a bad name. I'd expect that to be a file handle or
descriptor, not a string.
> - ok(SUCCEEDED(hr), "Failed to load texture (hr = %x).\n", hr);
> - ok(SUCCEEDED(hr), "Cannot get IDirect3DRMTexture interface (hr = %x)\n", hr);

This looks like it was leftover from somewhere, but in either case it
doesn't test what the message says it's testing.

It's fairly harmless here, but %x is a bad format in most cases. E.g.,
is "1000" decimal 4096 or decimal 1000? Just use %#x. Ideally you'd
also use the same message format as the other D3D tests.
> - check = DeleteFileA(file);
> - ok(check, "Cannot delete image stored in %s (error = %d).\n", file, GetLastError());
>
> +}

You're leaking "file".

[PATCH 2/2] d3drm/tests: Add QueryInterface tests for IDirect3DRMTexture*.

[Aaryaman Vasishta]

On Tue, Jan 19, 2016 at 3:09 AM, Henri Verbeet  wrote:
> On 18 January 2016 at 19:57, Aaryaman Vasishta
>  wrote:
> > - char *file = NULL;
>
> Why do you initialize this to NULL?

I don't remember exactly. I think it was to remove compiler warnings, but I
don't remember. I'll remove the NULL initialization if you want.
> > - BOOL check;
> > -
> > -
> > - hr = Direct3DRMCreate(&d3drm1);
>
> The extra empty line is weird. Is that intentional?

Not really, it's a silly mistake from me.
> It's fairly harmless here, but %x is a bad format in most cases. E.g.,
> is "1000" decimal 4096 or decimal 1000? Just use %#x. Ideally you'd
> also use the same message format as the other D3D tests.

Right, I'll keep a habit of using %#x from now on. :)

I'll resend the patch with the above fixes, including the rest of the
issues you mentioned.

Cheers,
Aaryaman