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 class="gmail_quote"><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 class="im">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>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>