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