On 11/04/18 20:50, Emil Velikov wrote:
On 10 April 2018 at 18:10, Jason Ekstrand <ja...@jlekstrand.net> wrote:
On Tue, Apr 10, 2018 at 10:05 AM, Emil Velikov <emil.l.veli...@gmail.com>
wrote:
On 10 April 2018 at 17:53, Ivan Kalvachev <ikalvac...@gmail.com> wrote:
On 3/28/18, Emil Velikov <emil.l.veli...@gmail.com> wrote:
From: Emil Velikov <emil.veli...@collabora.com>
Earlier commit enforced that we'll bail out if the number of
terminators
is different than 2. With that in mind, the assert() will never
trigger.
Fixes: 56b867395de ("glsl: fix infinite loop caused by bug in loop
unrolling pass")
This doesn't fix anything. Not triggering an assert is not a bug.
Removing a bogus assert that does get triggered by perfectly valid
code-paths would be a bug fix.
Cc: Timothy Arceri <tarc...@itsqueeze.com>
Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
Just a nitpick.
The explanations doesn't sound right to me.
Asserts are meant to never trigger.
They are used to check the internal logic of the code.
If this assert does trigger that would mean
that there is a bug in the code that makes sure
the number of terminators is different than 2.
It is better to catch bug with assert than
to silently do something wrong.
Right. wording is not perfect. As-is the assert provides misleading
assumption considering the explicit check
What misleading information would that be? In this particular case, we have
multiple cases of "if (term_count == 1) { ... } else { ... }" so knowing
that term_count never goes above 2 is useful. How is "assert(term_count <
2)" misleading?
In general, the point of asserts is to declare assumptions made by the code
that follows. This serves both as documentation to developers and ensures
that we find out if those assumptions are ever violated.
There is _explicit_ length check just before the loop. It should be
self-documenting enough, right?
if (... || ls->terminators.length() != 2)
return visit_continue;
That code was added with Tim's commit. I'm yet to see anybody adding
asserts for checking loop boundaries, hence the misleading label.
The better part is the existing assert did not flag prior to Tim's fix
(see the commit and bugreport).
Either way - if you feel strongly about it just revert it.
I'm fine with this being removed. We don't need an assert to check that
the if above succeeded. Also I wrote a piglit test to pick up the bug
that was fixed, so I'm not worried about regressions.
HTH
Emil
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
