Re: [PATCH] D24861: [Sema] extend Wshift-op-parentheses so it warns for multiplicative operators

2016-10-01 Thread Joerg Sonnenberger via cfe-commits
On Fri, Sep 30, 2016 at 03:41:53PM -0700, Richard Trieu via cfe-commits wrote: > Currently, this warning is on by default. As you said, the results you > found look intentional in many cases, so there is a high false positive > rate. For on by default warnings, we expect a high true positive

Re: [PATCH] D24861: [Sema] extend Wshift-op-parentheses so it warns for multiplicative operators

2016-09-30 Thread Richard Trieu via cfe-commits
Currently, this warning is on by default. As you said, the results you found look intentional in many cases, so there is a high false positive rate. For on by default warnings, we expect a high true positive rate and intend for users to not disable the warning. From my analysis on a separate

Re: [PATCH] D24861: [Sema] extend Wshift-op-parentheses so it warns for multiplicative operators

2016-09-28 Thread Daniel Marjamäki via cfe-commits
danielmarjamaki added a comment. I updated the patch so it does not warn about 'A * B << C'. It's a simple fix. I have not made careful measurements but I guess that the performance penalty is acceptable. https://reviews.llvm.org/D24861 ___

Re: [PATCH] D24861: [Sema] extend Wshift-op-parentheses so it warns for multiplicative operators

2016-09-28 Thread Daniel Marjamäki via cfe-commits
danielmarjamaki removed rL LLVM as the repository for this revision. danielmarjamaki updated this revision to Diff 72797. danielmarjamaki added a comment. Don't write warning for multiplication in LHS of <<. Often the execution order is not important. https://reviews.llvm.org/D24861 Files:

Re: [PATCH] D24861: [Sema] extend Wshift-op-parentheses so it warns for multiplicative operators

2016-09-27 Thread Joerg Sonnenberger via cfe-commits
joerg added a subscriber: joerg. joerg added a comment. I think the comment from Daniel shows the crux of the issue. A left shift is by nature a multiplication operation, so I don't see why it should get the warning. A right shift works like a division and order is quite significant for that.

Re: [PATCH] D24861: [Sema] extend Wshift-op-parentheses so it warns for multiplicative operators

2016-09-27 Thread David Blaikie via cfe-commits
I'd still wonder if this meets the bar for false positives (generally we try to only add warnings that would be enabled by default, even in new codebases - where most of what they find in a newcodebase are latent bugs, not noise (so the cleanup is at least fairly justified as being beneficial in

Re: [PATCH] D24861: [Sema] extend Wshift-op-parentheses so it warns for multiplicative operators

2016-09-27 Thread Bruno Cardoso Lopes via cfe-commits
bruno added a subscriber: bruno. bruno added a comment. Hi Daniel, This is very nice. In https://reviews.llvm.org/D24861#553606, @danielmarjamaki wrote: > Compiling 2064 projects resulted in 904 warnings > > Here are the results: >

Re: [PATCH] D24861: [Sema] extend Wshift-op-parentheses so it warns for multiplicative operators

2016-09-27 Thread Daniel Marjamäki via cfe-commits
danielmarjamaki added a comment. Compiling 2064 projects resulted in 904 warnings Here are the results: https://drive.google.com/file/d/0BykPmWrCOxt2N04tYl8zVHA3MXc/view?usp=sharing The results looks acceptable imho. The code looks intentional in many cases so I believe there are users that

Re: [PATCH] D24861: [Sema] extend Wshift-op-parentheses so it warns for multiplicative operators

2016-09-23 Thread David Blaikie via cfe-commits
Do you have some data on the true/false positive rate for this warning? On Fri, Sep 23, 2016 at 6:12 AM Daniel Marjamäki < daniel.marjam...@evidente.se> wrote: > danielmarjamaki created this revision. > danielmarjamaki added reviewers: dblaikie, rtrieu. > danielmarjamaki added a subscriber: