Re: [PATCH 1/3] d3dx9/tests: Add test for CommitChanges in effect.

[Matteo Bruni]

2017-03-09 23:15 GMT+01:00 Paul Gofman :
> Signed-off-by: Paul Gofman
>
> dlls/d3dx9_36/tests/effect.c | 1267 +++++++++++++++++++++++++++++++-----------
>  1 file changed, 930 insertions(+), 337 deletions(-)

First of all, please split up this patch, this is way too large and
does multiple things at once (I see that it extends the test effect
and checks the new states, factors out two helper functions from
test_effect_preshader() and it also adds the CommitChanges test in the
subject). Those are all good changes but when they are squeezed in one
patch it becomes hard to review.

I haven't looked in too much detail but anyway there are a few
additional comments below.
> +#define TEST_EFFECT_PRES_NOPTESTS ARRAY_SIZE(test_effect_preshader_op_results)

I know this isn't new but I don't think this define is very useful. If
you really want to keep it, please name it xxx_TEST_COUNT.
> -
>    else if (!state_updated[i])
> -
>    {
> -
>        todo_wine_if(memcmp(v, empty_v, sizeof(float) * 4))
> -
>        ok_(__FILE__, line)(!memcmp(v, empty_v, sizeof(float) * 4),
> -
>                "Parameter %s, test %d, operation %s, state updated unexpectedly.\n",
> -
>                updated_param, i, test_effect_preshader_op_results[i].comment);

Eh, this isn't really all that nice. I don't see an obvious way to
make this any better though so it probably makes sense to move this
part (or even all the tests) to the end of the patch series and avoid
the problem altogether.

BTW, if you define empty_v as an array you can use sizeof(empty_v) here.
> -
>    }
> - }
>
> +}
> -
>
> +static const D3DXVECTOR4 fvect_empty = {-9999.0f, -9999.0f, -9999.0f, -9999.0f};
> -
>
> +static void test_effect_preshader_clear_vconsts(IDirect3DDevice9 *device)
> +{
> - int i;

unsigned int i;
> - static const struct
> - {
> -
>    const char *param_name;
> -
>    BOOL const_updated[TEST_EFFECT_PRES_NFLOATV];
> - }
> - check_vconsts_parameters[] =
> - {
> -
>    {"g_Selector",
> -
>            {FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE,
> -
>             FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE,
> -
>             FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE,
> -
>             FALSE, FALSE, FALSE, TRUE}},

It would be nicer to represent these as 32-bit hexadecimal bitmasks.
> - for (i = 0; i < sizeof(check_op_parameters) / sizeof(*check_op_parameters); ++i)
> - {
> -
>    int j;

unsigned int j;
Also please leave a blank line between declarations and code.
> -
>    for (j = 0; j < 8; j++)

Consistency, please stick to the '++j' form.

[PATCH 1/3] d3dx9/tests: Add test for CommitChanges in effect.

[Paul Gofman]

Thanks, sure, I will do that.

On 03/14/2017 12:36 AM, Matteo Bruni wrote:
> 2017-03-09 23:15 GMT+01:00 Paul Gofman :
> > Signed-off-by: Paul Gofman
> >
> > dlls/d3dx9_36/tests/effect.c | 1267 +++++++++++++++++++++++++++++++-----------
> >   1 file changed, 930 insertions(+), 337 deletions(-)
>
> First of all, please split up this patch, this is way too large and
> does multiple things at once (I see that it extends the test effect
> and checks the new states, factors out two helper functions from
> test_effect_preshader() and it also adds the CommitChanges test in the
> subject). Those are all good changes but when they are squeezed in one
> patch it becomes hard to review.
>
> I haven't looked in too much detail but anyway there are a few
> additional comments below.
> > +#define TEST_EFFECT_PRES_NOPTESTS ARRAY_SIZE(test_effect_preshader_op_results)
>
> I know this isn't new but I don't think this define is very useful. If
> you really want to keep it, please name it xxx_TEST_COUNT.
> > -
> >    else if (!state_updated[i])
> > -
> >    {
> > -
> >        todo_wine_if(memcmp(v, empty_v, sizeof(float) * 4))
> > -
> >        ok_(__FILE__, line)(!memcmp(v, empty_v, sizeof(float) * 4),
> > -
> >                "Parameter %s, test %d, operation %s, state updated unexpectedly.\n",
> > -
> >                updated_param, i, test_effect_preshader_op_results[i].comment);
>
> Eh, this isn't really all that nice. I don't see an obvious way to
> make this any better though so it probably makes sense to move this
> part (or even all the tests) to the end of the patch series and avoid
> the problem altogether.
>
> BTW, if you define empty_v as an array you can use sizeof(empty_v) here.
> > -
> >    }
> > - }
> >
> > +}
> > -
> >
> > +static const D3DXVECTOR4 fvect_empty = {-9999.0f, -9999.0f, -9999.0f, -9999.0f};
> > -
> >
> > +static void test_effect_preshader_clear_vconsts(IDirect3DDevice9 *device)
> > +{
> > - int i;
>
> unsigned int i;
> > - static const struct
> > - {
> > -
> >    const char *param_name;
> > -
> >    BOOL const_updated[TEST_EFFECT_PRES_NFLOATV];
> > - }
> > - check_vconsts_parameters[] =
> > - {
> > -
> >    {"g_Selector",
> > -
> >            {FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE,
> > -
> >             FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE,
> > -
> >             FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE,
> > -
> >             FALSE, FALSE, FALSE, TRUE}},
>
> It would be nicer to represent these as 32-bit hexadecimal bitmasks.
> > - for (i = 0; i < sizeof(check_op_parameters) / sizeof(*check_op_parameters); ++i)
> > - {
> > -
> >    int j;
>
> unsigned int j;
> Also please leave a blank line between declarations and code.
> > -
> >    for (j = 0; j < 8; j++)
>
> Consistency, please stick to the '++j' form.