On 4/11/18, Ivan Kalvachev <ikalvac...@gmail.com> wrote: > On 4/11/18, Emil Velikov <emil.l.veli...@gmail.com> 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 see the root of the confusion. > > You see ( x !=2 ) and ( !(x<2) ) are not equivalent. > > Maybe the explicit check should be > if ( x >= 2 ) return;
Yff, the check makes sure the x is always 2. I should not write mails before morning cafe. Sorry for the noise. _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev