[PATCH] Warn on suspicious increments/decrements in for loops

[Richard Trieu]

Empty macros get expanded to a NullStmt, so the increment is not the last
statement in the loop body.


On Fri, May 31, 2013 at 3:49 PM, Richard Smith <richard at metafoo.co.uk>wrote:

> On Fri, May 31, 2013 at 3:38 PM, Richard Trieu <rtrieu at google.com> wrote:
>
>> On Fri, May 31, 2013 at 3:20 PM, Richard Smith <richard at metafoo.co.uk>wrote:
>>
>>> 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.
>>>
>>
>> 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.
>>
>
> Note that handle_field_3 is a macro which expands to nothing.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130531/cd88e419/attachment.html>