Re: [PATCH 5/8] d3dx9/tests: Factor out test_effect_preshader_compare_pbool_consts() function.

[Matteo Bruni]

2017-06-02 11:58 GMT+02:00 Paul Gofman :
> - if (!const_updated_mask)
> - {
> -
>    for (i = 0; i < ARRAY_SIZE(test_effect_preshader_bconsts); ++i)
> -
>    {
> -
>        ok_(__FILE__, line)(!bdata[i] == !test_effect_preshader_bconsts[i],
> -
>                "Pixel shader boolean constants do not match, expected %#x, got %#x, i %u.\n",
> -
>                test_effect_preshader_bconsts[i], bdata[i], i);
> -
>    }
> - }
> - else
> - {
> -
>    for (i = 0; i < ARRAY_SIZE(test_effect_preshader_bconsts); ++i)
> -
>    {
> -
>        if (const_updated_mask[i / TEST_EFFECT_BITMASK_BLOCK_SIZE]
> -
>                & (1u << (i % TEST_EFFECT_BITMASK_BLOCK_SIZE)))
> -
>        {
> -
>            ok_(__FILE__, line)(!bdata[i] == !test_effect_preshader_bconsts[i],
> -
>                    "Pixel shader boolean constants do not match, expected %#x, got %#x, i %u, parameter %s.\n",
> -
>                    test_effect_preshader_bconsts[i], bdata[i], i, updated_param);
> -
>        }
> -
>        else
> -
>        {
> -
>            ok_(__FILE__, line)(!bdata[i],
> -
>                    "Pixel shader boolean constants updated unexpectedly, parameter %s.\n", updated_param);
> -
>        }
> -
>    }
> - }

Minor simplification possibility: in the (!const_updated_mask) case
you could use a default "all ones" mask and use what's now the 'else'
branch, getting rid of the 'if' branch altogether.

[PATCH 5/8] d3dx9/tests: Factor out test_effect_preshader_compare_pbool_consts() function.

[Paul Gofman]

On 06/05/2017 07:55 PM, Matteo Bruni wrote:
> 2017-06-02 11:58 GMT+02:00 Paul Gofman :
> > - if (!const_updated_mask)
> > - {
> > -
> >    for (i = 0; i < ARRAY_SIZE(test_effect_preshader_bconsts); ++i)
> > -
> >    {
> > -
> >        ok_(__FILE__, line)(!bdata[i] == !test_effect_preshader_bconsts[i],
> > -
> >                "Pixel shader boolean constants do not match, expected %#x, got %#x, i %u.\n",
> > -
> >                test_effect_preshader_bconsts[i], bdata[i], i);
> > -
> >    }
> > - }
> > - else
> > - {
> > -
> >    for (i = 0; i < ARRAY_SIZE(test_effect_preshader_bconsts); ++i)
> > -
> >    {
> > -
> >        if (const_updated_mask[i / TEST_EFFECT_BITMASK_BLOCK_SIZE]
> > -
> >                & (1u << (i % TEST_EFFECT_BITMASK_BLOCK_SIZE)))
> > -
> >        {
> > -
> >            ok_(__FILE__, line)(!bdata[i] == !test_effect_preshader_bconsts[i],
> > -
> >                    "Pixel shader boolean constants do not match, expected %#x, got %#x, i %u, parameter %s.\n",
> > -
> >                    test_effect_preshader_bconsts[i], bdata[i], i, updated_param);
> > -
> >        }
> > -
> >        else
> > -
> >        {
> > -
> >            ok_(__FILE__, line)(!bdata[i],
> > -
> >                    "Pixel shader boolean constants updated unexpectedly, parameter %s.\n", updated_param);
> > -
> >        }
> > -
> >    }
> > - }
>
> Minor simplification possibility: in the (!const_updated_mask) case
> you could use a default "all ones" mask and use what's now the 'else'
> branch, getting rid of the 'if' branch altogether.

But I guess I should at least put the if on TRACE message, as without 
constant_update_mask there is no parameter name, that was the reason I 
left it the same way it was already done for vertex shader float 
constants. Should I put the if on TRACE instead?

[PATCH 5/8] d3dx9/tests: Factor out test_effect_preshader_compare_pbool_consts() function.

[Matteo Bruni]

2017-06-05 19:27 GMT+02:00 Paul Gofman :
> On 06/05/2017 07:55 PM, Matteo Bruni wrote:
> > 2017-06-02 11:58 GMT+02:00 Paul Gofman :
> > > - if (!const_updated_mask)
> > > - {
> > > -
> > >    for (i = 0; i < ARRAY_SIZE(test_effect_preshader_bconsts); ++i)
> > > -
> > >    {
> > > -
> > >        ok_(__FILE__, line)(!bdata[i] ==
> > >
> > > !test_effect_preshader_bconsts[i],
> > > -
> > >                "Pixel shader boolean constants do not match,
> > >
> > > expected %#x, got %#x, i %u.\n",
> > > -
> > >                test_effect_preshader_bconsts[i], bdata[i], i);
> > > -
> > >    }
> > > - }
> > > - else
> > > - {
> > > -
> > >    for (i = 0; i < ARRAY_SIZE(test_effect_preshader_bconsts); ++i)
> > > -
> > >    {
> > > -
> > >        if (const_updated_mask[i / TEST_EFFECT_BITMASK_BLOCK_SIZE]
> > > -
> > >                & (1u << (i % TEST_EFFECT_BITMASK_BLOCK_SIZE)))
> > > -
> > >        {
> > > -
> > >            ok_(__FILE__, line)(!bdata[i] ==
> > >
> > > !test_effect_preshader_bconsts[i],
> > > -
> > >                    "Pixel shader boolean constants do not match,
> > >
> > > expected %#x, got %#x, i %u, parameter %s.\n",
> > > -
> > >                    test_effect_preshader_bconsts[i], bdata[i], i,
> > >
> > > updated_param);
> > > -
> > >        }
> > > -
> > >        else
> > > -
> > >        {
> > > -
> > >            ok_(__FILE__, line)(!bdata[i],
> > > -
> > >                    "Pixel shader boolean constants updated
> > >
> > > unexpectedly, parameter %s.\n", updated_param);
> > > -
> > >        }
> > > -
> > >    }
> > > - }
> >
> > Minor simplification possibility: in the (!const_updated_mask) case
> > you could use a default "all ones" mask and use what's now the 'else'
> > branch, getting rid of the 'if' branch altogether.
>
> But I guess I should at least put the if on TRACE message, as without
> constant_update_mask there is no parameter name, that was the reason I left
> it the same way it was already done for vertex shader float constants.
> Should I put the if on TRACE instead?

That's not necessary, you can do e.g. updated_param ? updated_param : "".

No need to change the code now though, unless you're going to make
other changes in the area anyway.