[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 <gofmanp at gmail.com>:
>> Signed-off-by: Paul Gofman <gofmanp at gmail.com>
>> ---
>>   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.
>