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

[Ian Romanick]

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;

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 &=.

> -Emil
> _______________________________________________
> Piglit mailing list
> Piglit at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/piglit
>