Re: [PATCH 7/7] d3dx9/tests: Use static test blob in test_effect_preshader_ops().

[Matteo Bruni]

2017-05-19 13:56 GMT+02:00 Paul Gofman :

Thanks, I do think this is an improvement compared to the old tests. A
few comments below.
>  static const struct
>  {
> -
>    unsigned int pos;
> -
>    const char *mnem;
> -
>    unsigned int expected_result[4];
>    unsigned int result_index;
> -
>    unsigned int ins_count;
> -
>    float *result;
> -
>    D3DXVECTOR4 opvect1, opvect2, opvect3;
> -
>    unsigned int ulps;
> -
>    BOOL todo[4];
> }

I assume ulps and todo are there for future tests?
> -
>    for (j = 0; j < 4; ++j)
> -
>    {
> -
>        const float *v = op_tests[i].result;

Can you put the initialization of 'v' outside of the inner loop? I'm sure the
compiler sees the optimization and the generated code is ultimately
the same (not that it would really matter for a test anyway...) but it
seems a bit nicer to do that in the "correct" block.  You could also
call it "result" or similar.
> -
>        todo_wine_if(op_tests[i].todo[j])
> -
>        ok(compare_float(v[j], ((float *)op_tests[i].expected_result)[j], op_tests[i].ulps),
> -
>                "Operation %s, component %u, expected %#x (%g), got %#x (%g).\n", op_tests[i].mnem,
> -
>                j, op_tests[i].expected_result[j], ((float *)op_tests[i].expected_result)[j],
> -
>                ((unsigned int *)v)[j], v[j]);

Just an idea, maybe use %.8e to print the float values? Mostly for
consistency with the usual d3d stuff, although in this case it doesn't
matter much since you're also printing the bit encoding in hex.

Also you can clean the ok() a bit by defining a "const float
*expected_float;" variable (similarly to
test_effect_preshader_relative_addressing()). BTW, notice that you're
currently casting away const from v.

[PATCH 7/7] d3dx9/tests: Use static test blob in test_effect_preshader_ops().

[Paul Gofman]

On 05/22/2017 02:28 PM, Matteo Bruni wrote:
> 2017-05-19 13:56 GMT+02:00 Paul Gofman :
>
> Thanks, I do think this is an improvement compared to the old tests. A
> few comments below.
> >   static const struct
> >   {
> > -
> >    unsigned int pos;
> > -
> >    const char *mnem;
> > -
> >    unsigned int expected_result[4];
> >     unsigned int result_index;
> > -
> >    unsigned int ins_count;
> > -
> >    float *result;
> > -
> >    D3DXVECTOR4 opvect1, opvect2, opvect3;
> > -
> >    unsigned int ulps;
> > -
> >    BOOL todo[4];
> > }
>
> I assume ulps and todo are there for future tests?

Yes, they are not used right now. I left them in place because they 
already were in the structure and I thought it would be a bit strange to 
remove and add them now and then.
> > -
> >    for (j = 0; j < 4; ++j)
> > -
> >    {
> > -
> >        const float *v = op_tests[i].result;
>
> Can you put the initialization of 'v' outside of the inner loop? I'm sure the
> compiler sees the optimization and the generated code is ultimately
> the same (not that it would really matter for a test anyway...) but it
> seems a bit nicer to do that in the "correct" block.  You could also
> call it "result" or similar.

Yes, sure.
> > -
> >        todo_wine_if(op_tests[i].todo[j])
> > -
> >        ok(compare_float(v[j], ((float *)op_tests[i].expected_result)[j], op_tests[i].ulps),
> > -
> >                "Operation %s, component %u, expected %#x (%g), got %#x (%g).\n", op_tests[i].mnem,
> > -
> >                j, op_tests[i].expected_result[j], ((float *)op_tests[i].expected_result)[j],
> > -
> >                ((unsigned int *)v)[j], v[j]);
>
> Just an idea, maybe use %.8e to print the float values? Mostly for
> consistency with the usual d3d stuff, although in this case it doesn't
> matter much since you're also printing the bit encoding in hex.

I thought "%g" shows the values in a tiny bit more convenient way for 
viewing them (and as you pointed we have hex output for, e. g., copying 
the exact value from this output into the table). I actually found %g 
form of output handy when viewing these tests output. I have no problems 
changing it to ".8e" if you consider this better, but currently we are 
using "%g" throughout all the preshader tests, in this case I should 
probably do it throughout all the tests?
> Also you can clean the ok() a bit by defining a "const float
> *expected_float;" variable (similarly to
> test_effect_preshader_relative_addressing()). BTW, notice that you're
> currently casting away const from v.

I will do that.

[PATCH 7/7] d3dx9/tests: Use static test blob in test_effect_preshader_ops().

[Matteo Bruni]

2017-05-22 13:48 GMT+02:00 Paul Gofman :
> On 05/22/2017 02:28 PM, Matteo Bruni wrote:
> > Just an idea, maybe use %.8e to print the float values? Mostly for
> > consistency with the usual d3d stuff, although in this case it doesn't
> > matter much since you're also printing the bit encoding in hex.
>
> I thought "%g" shows the values in a tiny bit more convenient way for
> viewing them (and as you pointed we have hex output for, e. g., copying the
> exact value from this output into the table). I actually found %g form of
> output handy when viewing these tests output. I have no problems changing it
> to ".8e" if you consider this better, but currently we are using "%g"
> throughout all the preshader tests, in this case I should probably do it
> throughout all the tests?

Yes, ideally all the strings would need to be changed, although on the
other hand a patch just updating the ok() strings isn't particularly
appealing and mine is a very mild suggestion to begin with. It's up to
you, really.