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

[Emil Velikov]

On 23 November 2015 at 20:48, Ian Romanick  wrote:
> On 11/22/2015 03:43 AM, Emil Velikov wrote:
>> On 11 November 2015 at 18:07, Emil Velikov  wrote:
>>> From: Emil Velikov 
>>>
>>> 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 
>>> ---
>>>
>>> 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 : 
>>> >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.
>
"bikeshedding" wasn't used to belittle or stray anyone's input or
review about the patch. It was aimed to distinguish between the patch
from the comment after the --- line. As the former is (imho) dead
obvious, while the latter than provide an lengthy discussion.

>> 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
>
Admittedly I haven't looked that closely into piglits, but I have yet
to see a case like that.
Perhaps we can just add that is a big must/no-go and crack on ? We
might even want to check in-tree a coccinelle script to handle such
cases ?

> 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.
>
True, some of that can be attributed to slack review. Although there
was the argument "I saw this as the new format/used in mesa" which
albeit adequate, combined with people being busy on other things
doesn't make things any easier.

> 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 &=.
>
Fwiw I'm in no position to enforce either option, I'm just pointing
out that the issues in current patch, are not &= ones.

Perhaps regardless of the approach we choose, we should write some
coccinelle (other?) scripts to ensure/enforce 

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

[Ian Romanick]

On 11/23/2015 03:07 PM, Emil Velikov wrote:
> On 23 November 2015 at 20:48, Ian Romanick  wrote:
>> On 11/22/2015 03:43 AM, Emil Velikov wrote:
>>> On 11 November 2015 at 18:07, Emil Velikov  wrote:
 From: Emil Velikov 

 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 
 ---

 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 : 
 >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.
>>
> "bikeshedding" wasn't used to belittle or stray anyone's input or
> review about the patch. It was aimed to distinguish between the patch
> from the comment after the --- line. As the former is (imho) dead
> obvious, while the latter than provide an lengthy discussion.

I didn't think you were... that's why I said "we" instead of "you." :)
You did right by keeping the separate concerns separate.

>>> 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
>>
> Admittedly I haven't looked that closely into piglits, but I have yet
> to see a case like that.
> Perhaps we can just add that is a big must/no-go and crack on ? We
> might even want to check in-tree a coccinelle script to handle such
> cases ?

As Ilia pointed out (about the short circuiting), there may not be any.
 I remember that when Eric, Ken, and I started doing it this way, we
collectively had some good reasons.  It was 5+ years ago, and I guess I
don't actually remember what they were.  I know we considered both &=
and &&.

>> 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.
>>
> True, some of 

Re: [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  wrote:
> On 11/22/2015 03:43 AM, Emil Velikov wrote:
>> On 11 November 2015 at 18:07, Emil Velikov  wrote:
>>> From: Emil Velikov 
>>>
>>> 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 
>>> ---
>>>
>>> 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 : 
>>> >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
___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit

Re: [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  wrote:
>> From: Emil Velikov 
>>
>> 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 
>> ---
>>
>> 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 : 
>> >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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/piglit
> 

___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit

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

[Vinson Lee]

On Wed, Nov 11, 2015 at 10:07 AM, Emil Velikov  wrote:
> From: Emil Velikov 
>
> 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 
> ---
>
> 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/
>
> 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 : 
> >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++) {
> --
> 2.6.2
>
> ___
> Piglit mailing list
> Piglit@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/piglit

Reviewed-by: Vinson Lee 
___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit

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

[Emil Velikov]

On 11 November 2015 at 18:07, Emil Velikov  wrote:
> From: Emil Velikov 
>
> 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 
> ---
>
> 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/
>
> 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 : 
> >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 =
foo && pass" vs "pass &= foo" at a later stage.

-Emil
___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit