On Fri, May 31, 2013 at 3:38 PM, Richard Trieu <span dir="ltr"><<a href="mailto:rtrieu@google.com" target="_blank">rtrieu@google.com</a>></span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr"><div><div class="h5">On Fri, May 31, 2013 at 3:20 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br><div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>On Fri, May 31, 2013 at 2:46 PM, Richard Trieu <span dir="ltr"><<a href="mailto:rtrieu@google.com" target="_blank">rtrieu@google.com</a>></span> wrote:<br>

</div><div class="gmail_quote"><div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote"><div>On Fri, May 31, 2013 at 2:24 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br>



<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div>On Fri, May 31, 2013 at 2:04 PM, Richard Trieu <span dir="ltr"><<a href="mailto:rtrieu@google.com" target="_blank">rtrieu@google.com</a>></span> wrote:<br>



</div></div><div class="gmail_quote"><div><div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
  @jordan_rose, I want this warning.  Not sure about other people<br>
<br>
  @gribozavr, earlier versions of this did trigger on LLVM and Clang.  The warning has been fine-tuned since then to avoid those false positives.<br>
<br>
  Also, I seemed to have messed up the indentation when I wrote the visitors for the first -Wloop-analysis warning and managed to copy the bad indentation over to this change.  I will go fix them.</blockquote><div><br></div>




</div></div><div>Does this find any other bugs (or false positives) in other code you've run it on?</div></div></blockquote><div><br></div></div><div> This has found 15-20 bugs so far, with 1-2 false positives.  It is arguable that using (x+=2) in the loop header instead of two separate increments would be clearer for the code.</div>


</div></div></div></blockquote><div><br></div></div><div>15-20 bugs and 1-2 cases of code which is correct but unclear (and can trivially be rewritten to be correct and clear) sounds compelling to me (perhaps not for an enabled-by-default warning, but I think this meets the bar for -Wall -- we could really do with some published guidelines here).</div>


<div><br></div><div>Here's the most convincing form of false-positive I can come up with:</div><div><br></div><div>#define next_field(i) ++i</div><div>#define handle_field_3(x) /*nothing to do*/</div><div><br></div><div>


  for (int i = 0; i != record.size(); next_field(i)) {</div><div>    handle_field_1(record[i]);</div><div>    next_field(i);</div><div>    handle_field_2(record[i]);</div><div>    next_field(i);</div><div>    handle_field_3(record[i]);</div>


<div>  }</div><div><br></div><div>... but even here, the code would be clearer if the for-loop increment were moved into the loop body.</div><div><br></div><div>The patch itself looks fine, subject to prior comments.</div>


</div>
</blockquote></div><br></div></div></div><div class="gmail_extra">That is one of the two most common false positives I found when implementing this warning (the other involves continue statements).  To avoid warning on this pattern, it requires the increment to be the last statement of the body before triggering.  Since handle_field_3(record[i]) is the last statement, no warning gets emitted.</div>

</div>
</blockquote></div><br><div>Note that handle_field_3 is a macro which expands to nothing.</div>