[Sema] extend Wshift-op-parentheses so it warns for multiplicative operators

[danielmarjamaki]

Summary

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.

[danielmarjamaki]

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<<c) is evaluated first - unless there is some overflow or signedness issues. What do you think? I'll keep these warnings for now.

[bruno]

Hi Daniel,

This is very nice.

In 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<<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?

[joerg]

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.

[danielmarjamaki]

Don't write warning for multiplication in LHS of <<. Often the execution order is not important.

[danielmarjamaki]

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.

[danielmarjamaki]

Add new flag : -Wshift-op-parentheses-mul
This is not enabled by default.

[danielmarjamaki]

Ping

[danielmarjamaki]

I will not work on this in the near future

include/clang/Basic/DiagnosticSemaKinds.td

def warn_overloaded_shift_in_comparison :Warning<
   "comparison operator">,
   InGroup<OverloadedShiftOpParentheses>;
 def note_evaluate_comparison_first :Note<
   "place parentheses around comparison expression to evaluate it first">;
 
 def warn_addition_in_bitshift : Warning<
   "operator '%0' has lower precedence than '%1'; "
   "'%1' will be evaluated first">, InGroup<ShiftOpParentheses>;
+def warn_multiplication_in_bitshift : Warning<
+  "operator '%0' has lower precedence than '%1'; "
+  "'%1' will be evaluated first">, InGroup<DiagGroup<"shift-op-parentheses-mul">>, DefaultIgnore;
 
 def warn_self_assignment : Warning<
   "explicitly assigning value of variable of type %0 to itself">,
   InGroup<SelfAssignment>, DefaultIgnore;
 def warn_self_move : Warning<
   "explicitly moving variable of type %0 to itself">,
   InGroup<SelfMove>, DefaultIgnore;
 

lib/Sema/SemaExpr.cpp

if (Bop->isBitwiseOp() && Bop->getOpcode() < Opc) {
       SuggestParentheses(S, Bop->getOperatorLoc(),
         S.PDiag(diag::note_precedence_silence)
           << Bop->getOpcodeStr(),
         Bop->getSourceRange());
     }
   }
 }
 
-static void DiagnoseAdditionInShift(Sema &S, SourceLocation OpLoc,
-                                    Expr *SubExpr, StringRef Shift) {
-  if (BinaryOperator *Bop = dyn_cast<BinaryOperator>(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(),
+static void DiagnoseAddOrMulInShift(Sema &S, SourceLocation OpLoc,
+                                    Expr *SubExpr, StringRef Shift,
+                                    bool ShiftLeftLHS) {
+  BinaryOperator *Bop = dyn_cast<BinaryOperator>(SubExpr);
+  if (!Bop)
+    return;
+  if (!Bop->isAdditiveOp() && !Bop->isMultiplicativeOp())
+    return;
+  // In many cases, execution order does not matter for 'A*B<<C'.
+  if (ShiftLeftLHS && Bop->getOpcode() == BO_Mul)
+    return;
+
+  S.Diag(Bop->getOperatorLoc(), Bop->isAdditiveOp()
+                                    ? diag::warn_addition_in_bitshift
+                                    : diag::warn_multiplication_in_bitshift)
                      S.PDiag(diag::note_precedence_silence) << Op,
                      Bop->getSourceRange());
-    }
-  }
-}
 
 static void DiagnoseShiftCompare(Sema &S, SourceLocation OpLoc,
                                  Expr *LHSExpr, Expr *RHSExpr) {
   CXXOperatorCallExpr *OCE = dyn_cast<CXXOperatorCallExpr>(LHSExpr);
   if (!OCE)
     return;
 
   FunctionDecl *FD = OCE->getDirectCallee();
static void DiagnoseBinOpPrecedence(Sema &Self, BinaryOperatorKind Opc,
   if (Opc == BO_LOr && !OpLoc.isMacroID()/* Don't warn in macros. */) {
     DiagnoseLogicalAndInLogicalOrLHS(Self, OpLoc, LHSExpr, RHSExpr);
     DiagnoseLogicalAndInLogicalOrRHS(Self, OpLoc, LHSExpr, RHSExpr);
   }
 
   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:
   // cout << 5 == 4;
   if (BinaryOperator::isComparisonOp(Opc))
     DiagnoseShiftCompare(Self, OpLoc, LHSExpr, RHSExpr);
 }
 

test/Sema/parentheses.cpp

-// RUN: %clang_cc1 -Wparentheses -fsyntax-only -verify %s
-// RUN: %clang_cc1 -Wparentheses -fsyntax-only -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -Wparentheses -Wshift-op-parentheses-mul -fsyntax-only -verify %s
+// RUN: %clang_cc1 -Wparentheses -Wshift-op-parentheses-mul -fsyntax-only -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
 
 bool someConditionFunc();
 
 void conditional_op(int x, int y, bool b) {
   (void)(x + someConditionFunc() ? 1 : 2); // expected-warning {{operator '?:' has lower precedence than '+'}} \
                                            // expected-note {{place parentheses around the '+' expression to silence this warning}} \
                                            // expected-note {{place parentheses around the '?:' expression to evaluate it first}}
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:10-[[@LINE-3]]:10}:"("
void test(int a, int b, int c) {
   // 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]]: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}}
   // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:15-[[@LINE-2]]:15}:"("
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:20-[[@LINE-3]]:20}:")"
 }
 
 namespace PR15628 {