[PATCH] D24861: [Sema] extend Wshift-op-parentheses so it warns for multiplicative operators
David Blaikie via cfe-commits
cfe-commits at lists.llvm.org
Tue Sep 27 14:05:11 PDT 2016
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 itself rather than only as a means to enable the warning to catch some
bugs in the future))
But I know we made some exception for the &&/|| version of -Wparentheses,
so maybe this variation meets that same bar. (if Richard Trieu doesn't have
enough context to make that call, I'd probably rope in Richard Smith (&
possibly Chandler Carruth - I recall him commenting on the &&/||
-Wparentheses form on more than one occasion))
On Tue, Sep 27, 2016 at 10:01 AM Bruno Cardoso Lopes <
bruno.cardoso at gmail.com> wrote:
> 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:
> > The results looks acceptable imho. The code looks intentional in many
> cases so I believe there are users that will disable this warning. Probably
> there are true positives where the evaluation order is not really known.
> There were many warnings about macro arguments where the macro bitshifts
> the argument - these macros look very shaky to me.
> > I saw some warnings about such code:
> > a * b << c
> > Maybe we should not warn about this. As far as I see, the result will be
> the same if (a*b) or (b<<c) is evaluated first - unless there is some
> overflow or signedness issues. What do you think? I'll keep these warnings
> for now.
> Any idea on how expensive would be to reason about these false positives
> and avoid them?
> rL LLVM
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the cfe-commits