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

[Emil Velikov]

On 10 April 2018 at 18:10, Jason Ekstrand <jason at jlekstrand.net> wrote:
> 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.

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.

HTH
Emil