[Mesa-dev] [PATCH] glsl: remove unreachable assert()

[Jason Ekstrand]

On Tue, Apr 10, 2018 at 10:05 AM, Emil Velikov <emil.l.velikov at gmail.com>
wrote:

> On 10 April 2018 at 17:53, Ivan Kalvachev <ikalvachev at gmail.com> wrote:
> > On 3/28/18, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> >> From: Emil Velikov <emil.velikov at 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 <tarceri at itsqueeze.com>
> >> Signed-off-by: Emil Velikov <emil.velikov at 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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180410/b324dca4/attachment.html>