[Piglit] [PATCH] texwrap: do no short circuit remaining tests if one fails

[Ilia Mirkin]

On Mon, Nov 23, 2015 at 3:48 PM, Ian Romanick <idr at freedesktop.org> wrote:
> On 11/22/2015 03:43 AM, Emil Velikov wrote:
>> On 11 November 2015 at 18:07, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>>> From: Emil Velikov <emil.velikov at collabora.com>
>>>
>>> Noticed as some of these have been intermittently failing on llvmpipe,
>>> resulting in a few "not run" test across mesa release checks.
>>>
>>> Signed-off-by: Emil Velikov <emil.velikov at collabora.com>
>>> ---
>>>
>>> XXX:
>>> At some point we'd want to do a tree-wide:
>>>  - s/GLboolean pass/bool pass/
>>>  - s/pass = pass && foo/pass &= foo/
>>>  - s/pass = foo && pass/pass &= foo/
>
> Yes, please... but see below.
>
>>> We might want to convert the test to use the piglit_probe_pixels over
>>> it's custom ones.
>>>
>>> -Emil
>>>
>>>  tests/texturing/texwrap.c | 6 +++---
>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/tests/texturing/texwrap.c b/tests/texturing/texwrap.c
>>> index fbe9068..60ffa73 100644
>>> --- a/tests/texturing/texwrap.c
>>> +++ b/tests/texturing/texwrap.c
>>> @@ -1134,7 +1134,7 @@ static GLboolean test_format_npot(const struct format_desc *format, GLboolean np
>>>                  * It has to be enabled on the command line.
>>>                  */
>>>                 if (!texture_swizzle && !npot && !test_border_color && has_texture_swizzle) {
>>> -                       pass = pass && test_format_npot_swizzle(format, npot, 1);
>>> +                       pass = test_format_npot_swizzle(format, npot, 1) && pass;
>>>                 }
>>>         }
>>>         return pass;
>>> @@ -1149,7 +1149,7 @@ static GLboolean test_format(const struct format_desc *format)
>>>         } else {
>>>                 pass = test_format_npot(format, 0);
>>>                 if (has_npot && !test_border_color) {
>>> -                       pass = pass && test_format_npot(format, 1);
>>> +                       pass = test_format_npot(format, 1) && pass;
>>>                 }
>>>         }
>>>         return pass;
>>> @@ -1163,7 +1163,7 @@ enum piglit_result piglit_display()
>>>                 pass = test_format(init_format ? init_format : &test->format[0]);
>>>         } else {
>>>                 if (init_format) {
>>> -                       pass = pass && test_format(init_format);
>>> +                       pass = test_format(init_format) && pass;
>>>                 } else {
>>>                         int i;
>>>                         for (i = 0; i < test->num_formats; i++) {
>>> --
>> Any takers on this trivial patch ? I guess we can bikeshed the "pass =
>
> I don't think we should use the term bikeshed.  While it didn't start
> that way, it has become a purely pejorative term used to dismiss
> someone's feedback.  It's only purpose these days seems to be to make
> people mad and start arguments.
>
>> foo && pass" vs "pass &= foo" at a later stage.
>
> The problem with &= is that it doesn't extend to more than two
> predicates.  As a result, there will always be place in piglit that do
>
>     pass = foo &&
>            bar &&
>            asdf &&
>            pass;
>
> We don't want to have two different idioms for essentially the same
> thing.  That means that test authors and reviews have to stop and think
> about which idiom should be used in which places.  It also means that if
> a place that used &= is extended with another predicate, you have to
> change more of the code. So,
>
>     pass &= foo;
>
> would become
>
>     pass = foo &&
>            bar &&
>            pass;

This would end up short-curcuiting if foo were false (probably not the
intent), and would work just as well as

pass &= foo && bar;

If you didn't want the short-circuit, you could instead do

pass &= foo;
pass &= bar;

>
> And everyone has to review it more carefully to be sure it's right.
>
> Moreover, "pass = foo && pass;" has been the piglit idiom for *years*.
> A few new-comers came along and started using the other idiom because
> they had used it on other projects.  Piglit's slack review requirement
> allowed a bunch of that to slip in.
>
> I really don't want to see it spread any further, and I'd be happy to
> review patches that change the existing uses of &=.

IMHO it's a lot easier to get &= right than making sure that things
don't end up getting accidentally short-cicruited.

  -ilia