d3dx9: Add ID3DXSprite tests (review please)

[Vitaliy Margolen]

Tony Wasserka wrote:
> This is my first test, so I think it's better to ask for feedback on
> wine-devel, first.
> 

In all of your patch:
> +    if(!obj) return 0;
Please add space after "if" to make it consistent and more readable:
    if (!obj) return 0;

> hr=IDirect3DDevice9_CreateTexture
Please add spaces to the both sides of "=".



> +#define CHECK_REF(obj, exp) \
> +        ok(exp==get_ref((IUnknown*)(obj)), "Invalid refcount. Expected %d, got %d\n", exp, get_ref((IUnknown*)(obj)));
You should make it an inine perhaps to not call get_ref twice on the same
object.

> +#define CHECK_RELEASE(obj, exp) \
> +    { \
> +        if(obj) { \
> +            int ref=IUnknown_Release((IUnknown*)(obj)); \
> +            ok(ref==exp, "Invalid refcount. Expected %d, got %d\n", exp, ref); \
> +        } else skip("Object was NULL before Release\n"); \
> +    }
This also should be a function. And please drop skip - it should only be
used to skip parts of the test. Here you just skipping one check which
should always be valid.

> +#define CHECK_CALL(call, name, exp) \
> +    hr=(call); \
> +    ok(hr==(exp), "%s returned %#x, expected %#x\n", (name), hr, (exp));
> +
Don't use macros like this. Put it all in the code. It's bad thing to have
macro modify some variables (hr) you don't explicitly specify as params or
return.

> +    ID3DXSprite *sprite=NULL;
> +    IDirect3DDevice9 *cmpdev=NULL;
> +    IDirect3DTexture9 *tex1=NULL, *tex2=NULL;
No need to initialize variables that you always assign to.

> +    if(FAILED(hr=IDirect3DDevice9_CreateTexture(device, 64, 64, 1, 0, D3DFMT_A8R8G8B8, D3DPOOL_DEFAULT, &tex1, NULL))) {
> +        skip("Couldn't create first texture, CreateTexture returned %#x\n", hr);
> +        return;
> +    }
Failure to create a texture should be considered a test - use ok(). And
please put hr=IDirect3DDevice9_CreateTexture() on the separate line.

> +    if(SUCCEEDED(hr)) {
> +        D3DXMATRIX identity;
> +        D3DXMatrixIdentity(&identity);
> +        check_mat(mat, identity);
> +    } else skip("No matrix to test\n");
No need to use skip() here.


Vitaliy.