Re: [PATCH 1/6] d3dx9_36/tests: Added HLSL test suite.

[Henri Verbeet]

On 5 August 2010 00:05, Travis Athougies <iammisc(a)gmail.com> wrote:
> +#define CINTERFACE
> Why do you need this?
> +#include <d3d9.h>
> d3d9.h should be implied by d3dx9.h
> +#include <assert.h>
> +#include <stdio.h>
> I don't think you need these either.
> +struct vertex
> +{
> +    float x, y, z;
> +    float tx, ty;
> +};
> +
> +const struct vertex quad_vertices[4] = {
> +    {-1, -1, 0, 0, 1},
> +    {-1, 1, 0, 0, 0},
> +    {1, -1, 0, 1, 1},
> +    {1, 1, 0, 1, 0}
> +};
> Those aren't floating point values. Also, please use consistent formatting.
> +const struct vertex quad_vertices[4] = {
> ...
> +static IDirect3DVertexBuffer9 *quad_geometry;
> +static IDirect3DVertexShader9 *vshader_passthru;
> +static IDirect3DVertexDeclaration9 *vdeclaration;
> +static IDirect3DDevice9 *device;
> Why is all of that global?
> +    hres = IDirect3D9_CreateDevice(d3d9_ptr, D3DADAPTER_DEFAULT, D3DDEVTYPE_HAL, NULL, D3DCREATE_HARDWARE_VERTEXPROCESSING, &present_parameters, &device_ptr);
> Please break long lines. I realize this particular one is probably
> somehow my fault because I added it back in 2005 with
> c26ad82e75e408c302c21228ef08c1af0482326c, and everyone has been
> (blindly) copying it since, but there are other examples like
> setup_device() as well. Use 120 as maximum line length, 100 to be
> safe.
> +    hres = IDirect3DVertexBuffer9_Lock(quad_geometry, 0, sizeof(quad_vertices), (void**)&temp_geometry_vertices, 0);
> You don't need that cast.
> +    hres = D3DXCompileShader(vshader_passthru_hlsl, strlen(vshader_passthru_hlsl),
> +                             NULL, NULL, "vshader", "vs_1_1", 0,
> +                             &compiled, &errors, 0);
> +    if(FAILED(hres))
> +    {
> +        vshader_passthru = NULL;
> That doesn't make sense, vshader_passthru is already NULL at that point.
> +    hr = D3DXCompileShader(shader, strlen(shader),
> +                           NULL, NULL, "test", profile,
> +                           0, &compiled, &errors, constants);
> +    ok (hr == D3D_OK, "Pixel shader %s compilation failed: %s\n", shader, (char*) (errors?ID3DXBuffer_GetBufferPointer(errors):""));
> This casts away const.
> diff --git a/include/d3dx9shader.h b/include/d3dx9shader.h
> index c8380bc..c54d27c 100644
> --- a/include/d3dx9shader.h
> +++ b/include/d3dx9shader.h
> @@ -296,6 +296,17 @@ HRESULT WINAPI D3DXAssembleShader(LPCSTR data,
>                                   LPD3DXBUFFER* shader,
>                                   LPD3DXBUFFER* error_messages);
>
> +HRESULT WINAPI D3DXCompileShader(LPCSTR src_data,
> +                                 UINT data_len,
> +                                 const D3DXMACRO* defines,
> +                                 LPD3DXINCLUDE include,
> +                                 LPCSTR function_name,
> +                                 LPCSTR profile,
> +                                 DWORD flags,
> +                                 LPD3DXBUFFER* shader,
> +                                 LPD3DXBUFFER* error_messages,
> +                                 LPD3DXCONSTANTTABLE* constant_table);
> +
> This should be in a separate patch.

The patch has some more issues, they shouldn't be hard to spot.