On 11/23/2015 03:07 PM, Emil Velikov wrote: > On 23 November 2015 at 20:48, Ian Romanick <i...@freedesktop.org> wrote: >> On 11/22/2015 03:43 AM, Emil Velikov wrote: >>> On 11 November 2015 at 18:07, Emil Velikov <emil.l.veli...@gmail.com> wrote: >>>> From: Emil Velikov <emil.veli...@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.veli...@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. >> > "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 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 things. Sort of like the > kernel's check-patch - always run your patches through it and fix the > issues before sending upstream. Perhaps we can also use check-patch > with a few piglit specific tweaks ? > > Shame Coccinelle skills are non existent :-\ > > -Emil _______________________________________________ Piglit mailing list Piglit@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/piglit