[PATCH] Warn on suspicious increments/decrements in for loops
Richard Smith
richard at metafoo.co.uk
Fri May 31 15:20:13 PDT 2013
On Fri, May 31, 2013 at 2:46 PM, Richard Trieu <rtrieu at google.com> wrote:
>
>
>
> On Fri, May 31, 2013 at 2:24 PM, Richard Smith <richard at metafoo.co.uk>wrote:
>
>> On Fri, May 31, 2013 at 2:04 PM, Richard Trieu <rtrieu at google.com> wrote:
>>
>>>
>>> @jordan_rose, I want this warning. Not sure about other people
>>>
>>> @gribozavr, earlier versions of this did trigger on LLVM and Clang.
>>> The warning has been fine-tuned since then to avoid those false positives.
>>>
>>> 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.
>>
>>
>> Does this find any other bugs (or false positives) in other code you've
>> run it on?
>>
>
> 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.
>
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).
Here's the most convincing form of false-positive I can come up with:
#define next_field(i) ++i
#define handle_field_3(x) /*nothing to do*/
for (int i = 0; i != record.size(); next_field(i)) {
handle_field_1(record[i]);
next_field(i);
handle_field_2(record[i]);
next_field(i);
handle_field_3(record[i]);
}
... but even here, the code would be clearer if the for-loop increment were
moved into the loop body.
The patch itself looks fine, subject to prior comments.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130531/0d33716f/attachment.html>
More information about the cfe-commits
mailing list