<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Apr 10, 2018 at 10:05 AM, Emil Velikov <span dir="ltr"><<a href="mailto:emil.l.velikov@gmail.com" target="_blank">emil.l.velikov@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On 10 April 2018 at 17:53, Ivan Kalvachev <<a href="mailto:ikalvachev@gmail.com">ikalvachev@gmail.com</a>> wrote:<br>
> On 3/28/18, Emil Velikov <<a href="mailto:emil.l.velikov@gmail.com">emil.l.velikov@gmail.com</a>> wrote:<br>
>> From: Emil Velikov <<a href="mailto:emil.velikov@collabora.com">emil.velikov@collabora.com</a>><br>
>><br>
>> Earlier commit enforced that we'll bail out if the number of terminators<br>
>> is different than 2. With that in mind, the assert() will never trigger.<br>
>><br>
>> Fixes: 56b867395de ("glsl: fix infinite loop caused by bug in loop<br>
>> unrolling pass")<br></span></blockquote><div><br>This doesn't fix anything.  Not triggering an assert is not a bug.<br><br>Removing a bogus assert that does get triggered by perfectly valid code-paths would be a bug fix.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
>> Cc: Timothy Arceri <<a href="mailto:tarceri@itsqueeze.com">tarceri@itsqueeze.com</a>><br>
>> Signed-off-by: Emil Velikov <<a href="mailto:emil.velikov@collabora.com">emil.velikov@collabora.com</a>><br>
><br>
> Just a nitpick.<br>
> The explanations doesn't sound right to me.<br>
><br>
> Asserts are meant to never trigger.<br>
> They are used to check the internal logic of the code.<br>
><br>
> If this assert does trigger that would mean<br>
> that there is a bug in the code that makes sure<br>
> the number of terminators is different than 2.<br>
><br>
> It is better to catch bug with assert than<br>
> to silently do something wrong.<br>
><br>
</span>Right. wording is not perfect. As-is the assert provides misleading<br>
assumption considering the explicit check</blockquote><div><br></div><div>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?<br><br></div><div>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.<br></div></div></div></div>