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