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

[Timothy Arceri]

On 11/04/18 20:50, Emil Velikov wrote:
> 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.

I'm fine with this being removed. We don't need an assert to check that 
the if above succeeded. Also I wrote a piglit test to pick up the bug 
that was fixed, so I'm not worried about regressions.

>
> HTH
> Emil