On Fri, May 31, 2013 at 3:38 PM, Richard Trieu <[email protected]> wrote:
> On Fri, May 31, 2013 at 3:20 PM, Richard Smith <[email protected]>wrote: > >> On Fri, May 31, 2013 at 2:46 PM, Richard Trieu <[email protected]> wrote: >> >>> >>> >>> >>> On Fri, May 31, 2013 at 2:24 PM, Richard Smith <[email protected]>wrote: >>> >>>> On Fri, May 31, 2013 at 2:04 PM, Richard Trieu <[email protected]>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.
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
