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

[Ivan Kalvachev]

On 4/11/18, Emil Velikov <emil.l.velikov at gmail.com> 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 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.