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

[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 rate and
> intend for users to not disable the warning.  From my analysis on a
> separate codebase, I found less than 10% true positive rate out of 200
> warnings.  One option might be to move this warning to a subgroup, which
> would leave it discoverable from either -Wall or -Wparentheses, but not
> have it on by default.

We are now only talking about the right shift version, are we? That
seems to me to be much less intrusive, but should belong into the same
subgroup as the add checks.

Joerg
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[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 codebase, I found less than 10% true positive rate out of 200
warnings.  One option might be to move this warning to a subgroup, which
would leave it discoverable from either -Wall or -Wparentheses, but not
have it on by default.

On Wed, Sep 28, 2016 at 4:09 AM, Daniel Marjamäki <
daniel.marjam...@evidente.se> wrote:

> 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
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[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:
  lib/Sema/SemaExpr.cpp
  test/Sema/parentheses.cpp

Index: test/Sema/parentheses.cpp
===
--- test/Sema/parentheses.cpp
+++ test/Sema/parentheses.cpp
@@ -95,6 +95,23 @@
   // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:10-[[@LINE-2]]:10}:"("
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:15-[[@LINE-3]]:15}:")"
 
+  (void)(a >> b * c); // expected-warning {{operator '>>' has lower precedence 
than '*'; '*' will be evaluated first}} \
+ expected-note {{place parentheses around the '*' 
expression to silence this warning}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:15-[[@LINE-2]]:15}:"("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:20-[[@LINE-3]]:20}:")"
+
+  (void)(a / b << c); // expected-warning {{operator '<<' has lower precedence 
than '/'; '/' will be evaluated first}} \
+ expected-note {{place parentheses around the '/' 
expression to silence this warning}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:10-[[@LINE-2]]:10}:"("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:15-[[@LINE-3]]:15}:")"
+
+  (void)(a % b << c); // expected-warning {{operator '<<' has lower precedence 
than '%'; '%' will be evaluated first}} \
+ expected-note {{place parentheses around the '%' 
expression to silence this warning}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:10-[[@LINE-2]]:10}:"("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:15-[[@LINE-3]]:15}:")"
+
+  (void)(a * b << c); // no warning, often the execution order does not matter.
+
   Stream() << b + c;
   Stream() >> b + c; // expected-warning {{operator '>>' has lower precedence 
than '+'; '+' will be evaluated first}} \
 expected-note {{place parentheses around the '+' 
expression to silence this warning}}
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -11246,18 +11246,24 @@
   }
 }
 
-static void DiagnoseAdditionInShift(Sema , SourceLocation OpLoc,
-Expr *SubExpr, StringRef Shift) {
-  if (BinaryOperator *Bop = dyn_cast(SubExpr)) {
-if (Bop->getOpcode() == BO_Add || Bop->getOpcode() == BO_Sub) {
-  StringRef Op = Bop->getOpcodeStr();
-  S.Diag(Bop->getOperatorLoc(), diag::warn_addition_in_bitshift)
-  << Bop->getSourceRange() << OpLoc << Shift << Op;
-  SuggestParentheses(S, Bop->getOperatorLoc(),
-  S.PDiag(diag::note_precedence_silence) << Op,
-  Bop->getSourceRange());
-}
-  }
+static void DiagnoseAddOrMulInShift(Sema , SourceLocation OpLoc,
+Expr *SubExpr, StringRef Shift,
+bool ShiftLeftLHS) {
+  BinaryOperator *Bop = dyn_cast(SubExpr);
+  if (!Bop)
+return;
+  if (!Bop->isAdditiveOp() && !Bop->isMultiplicativeOp())
+return;
+  // In many cases, execution order does not matter for 'A*B<getOpcode() == BO_Mul)
+return;
+
+  StringRef Op = Bop->getOpcodeStr();
+  S.Diag(Bop->getOperatorLoc(), diag::warn_addition_in_bitshift)
+  << Bop->getSourceRange() << OpLoc << Shift << Op;
+  SuggestParentheses(S, Bop->getOperatorLoc(),
+ S.PDiag(diag::note_precedence_silence) << Op,
+ Bop->getSourceRange());
 }
 
 static void DiagnoseShiftCompare(Sema , SourceLocation OpLoc,
@@ -11313,8 +11319,8 @@
   if ((Opc == BO_Shl && 
LHSExpr->getType()->isIntegralType(Self.getASTContext()))
   || Opc == BO_Shr) {
 StringRef Shift = BinaryOperator::getOpcodeStr(Opc);
-DiagnoseAdditionInShift(Self, OpLoc, LHSExpr, Shift);
-DiagnoseAdditionInShift(Self, OpLoc, RHSExpr, Shift);
+DiagnoseAddOrMulInShift(Self, OpLoc, LHSExpr, Shift, Opc == BO_Shl);
+DiagnoseAddOrMulInShift(Self, OpLoc, RHSExpr, Shift, false);
   }
 
   // Warn on overloaded shift operators and comparisons, such as:


Index: test/Sema/parentheses.cpp
===
--- test/Sema/parentheses.cpp
+++ test/Sema/parentheses.cpp
@@ -95,6 +95,23 @@
   // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:10-[[@LINE-2]]:10}:"("
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:15-[[@LINE-3]]:15}:")"
 
+  (void)(a >> b * c); // expected-warning {{operator '>>' has lower precedence than '*'; '*' will be evaluated first}} \
+ expected-note {{place parentheses around the '*' expression to silence this warning}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:15-[[@LINE-2]]:15}:"("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:20-[[@LINE-3]]:20}:")"
+
+  (void)(a / b << c); // expected-warning {{operator '<<' 

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

[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.


Repository:
  rL LLVM

https://reviews.llvm.org/D24861



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[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 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.card...@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:
> >
> 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 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< 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?
>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D24861
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[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:
>  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 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< 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?


Repository:
  rL LLVM

https://reviews.llvm.org/D24861



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[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 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<

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

[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: cfe-commits.
> danielmarjamaki set the repository for this revision to rL LLVM.
>
> This patch makes Clang warn about following code:
>
> a = (b * c >> 2);
>
> It might be a good idea to use parentheses to clarify the operator
> precedence.
>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D24861
>
> Files:
>   lib/Sema/SemaExpr.cpp
>   test/Sema/parentheses.cpp
>
> Index: test/Sema/parentheses.cpp
> ===
> --- test/Sema/parentheses.cpp
> +++ test/Sema/parentheses.cpp
> @@ -95,6 +95,21 @@
>// CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:10-[[@LINE-2]]:10}:"("
>// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:15-[[@LINE-3]]:15}:")"
>
> +  (void)(a >> b * c); // expected-warning {{operator '>>' has lower
> precedence than '*'; '*' will be evaluated first}} \
> + expected-note {{place parentheses around the '*'
> expression to silence this warning}}
> +  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:15-[[@LINE-2]]:15}:"("
> +  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:20-[[@LINE-3]]:20}:")"
> +
> +  (void)(a / b << c); // expected-warning {{operator '<<' has lower
> precedence than '/'; '/' will be evaluated first}} \
> + expected-note {{place parentheses around the '/'
> expression to silence this warning}}
> +  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:10-[[@LINE-2]]:10}:"("
> +  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:15-[[@LINE-3]]:15}:")"
> +
> +  (void)(a % b << c); // expected-warning {{operator '<<' has lower
> precedence than '%'; '%' will be evaluated first}} \
> + expected-note {{place parentheses around the '%'
> expression to silence this warning}}
> +  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:10-[[@LINE-2]]:10}:"("
> +  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:15-[[@LINE-3]]:15}:")"
> +
>Stream() << b + c;
>Stream() >> b + c; // expected-warning {{operator '>>' has lower
> precedence than '+'; '+' will be evaluated first}} \
>  expected-note {{place parentheses around the '+'
> expression to silence this warning}}
> Index: lib/Sema/SemaExpr.cpp
> ===
> --- lib/Sema/SemaExpr.cpp
> +++ lib/Sema/SemaExpr.cpp
> @@ -11246,10 +11246,10 @@
>}
>  }
>
> -static void DiagnoseAdditionInShift(Sema , SourceLocation OpLoc,
> +static void DiagnoseAddOrMulInShift(Sema , SourceLocation OpLoc,
>  Expr *SubExpr, StringRef Shift) {
>if (BinaryOperator *Bop = dyn_cast(SubExpr)) {
> -if (Bop->getOpcode() == BO_Add || Bop->getOpcode() == BO_Sub) {
> +if (Bop->isAdditiveOp() || Bop->isMultiplicativeOp()) {
>StringRef Op = Bop->getOpcodeStr();
>S.Diag(Bop->getOperatorLoc(), diag::warn_addition_in_bitshift)
><< Bop->getSourceRange() << OpLoc << Shift << Op;
> @@ -11313,8 +11313,8 @@
>if ((Opc == BO_Shl &&
> LHSExpr->getType()->isIntegralType(Self.getASTContext()))
>|| Opc == BO_Shr) {
>  StringRef Shift = BinaryOperator::getOpcodeStr(Opc);
> -DiagnoseAdditionInShift(Self, OpLoc, LHSExpr, Shift);
> -DiagnoseAdditionInShift(Self, OpLoc, RHSExpr, Shift);
> +DiagnoseAddOrMulInShift(Self, OpLoc, LHSExpr, Shift);
> +DiagnoseAddOrMulInShift(Self, OpLoc, RHSExpr, Shift);
>}
>
>// Warn on overloaded shift operators and comparisons, such as:
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits