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.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to