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

[Ivan Kalvachev]

On 4/11/18, Ivan Kalvachev <ikalvachev at gmail.com> wrote:
> 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;

Yff, the check makes sure the x is always 2.

I should not write mails before morning cafe.

Sorry for the noise.