Implement -Wshift-op-parentheses for: a << b + c

This appears to be consistent with GCC's implementation of the same warning
under -Wparentheses. Suppressing a << b + c for cases where 'a' is a user
defined type for compatibility with C++ stream IO. Otherwise suggest
parentheses around the addition or subtraction subexpression.

(this came up when MSVC was complaining (incorrectly, so far as I can tell)
about a perceived violation of this within the LLVM codebase, PR14001)

llvm-svn: 165283

4 files changed, 39 insertions, 0 deletions

diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index 156cf54d706..e5e9021e653 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -119,6 +119,7 @@ def GlobalConstructors : DiagGroup<"global-constructors">;
 def : DiagGroup<"idiomatic-parentheses">;
 def BitwiseOpParentheses: DiagGroup<"bitwise-op-parentheses">;
 def LogicalOpParentheses: DiagGroup<"logical-op-parentheses">;
+def ShiftOpParentheses: DiagGroup<"shift-op-parentheses">;
 def DanglingElse: DiagGroup<"dangling-else">;
 def IgnoredQualifiers : DiagGroup<"ignored-qualifiers">;
 def : DiagGroup<"import">;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 0f0f4f13ebe..d8893311521 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3863,6 +3863,11 @@ def warn_logical_and_in_logical_or : Warning<
 def note_logical_and_in_logical_or_silence : Note<
   "place parentheses around the '&&' expression to silence this warning">;
 
+def warn_addition_in_bitshift : Warning<
+  "'%0' within '%1'">, InGroup<ShiftOpParentheses>;
+def note_addition_in_bitshift_silence : Note<
+  "place parentheses around the '%0' expression to silence this warning">;
+
 def warn_self_assignment : Warning<
   "explicitly assigning a variable of type %0 to itself">,
   InGroup<SelfAssignment>, DefaultIgnore;
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index bbae55b5980..d40818b83b0 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -8570,6 +8570,20 @@ static void DiagnoseBitwiseAndInBitwiseOr(Sema &S, SourceLocation OpLoc,
   }
 }
 
+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->getOpcode() == BO_Add ? "+" : "-";
+      S.Diag(Bop->getOperatorLoc(), diag::warn_addition_in_bitshift)
+          << Bop->getSourceRange() << OpLoc << op << shift;
+      SuggestParentheses(S, Bop->getOperatorLoc(),
+          S.PDiag(diag::note_addition_in_bitshift_silence) << op,
+          Bop->getSourceRange());
+    }
+  }
+}
+
 /// DiagnoseBinOpPrecedence - Emit warnings for expressions with tricky
 /// precedence.
 static void DiagnoseBinOpPrecedence(Sema &Self, BinaryOperatorKind Opc,
@@ -8591,6 +8605,13 @@ static void DiagnoseBinOpPrecedence(Sema &Self, BinaryOperatorKind Opc,
     DiagnoseLogicalAndInLogicalOrLHS(Self, OpLoc, LHSExpr, RHSExpr);
     DiagnoseLogicalAndInLogicalOrRHS(Self, OpLoc, LHSExpr, RHSExpr);
   }
+
+  if ((Opc == BO_Shl && LHSExpr->getType()->isIntegralType(Self.getASTContext()))
+      || Opc == BO_Shr) {
+    StringRef shift = Opc == BO_Shl ? "<<" : ">>";
+    DiagnoseAdditionInShift(Self, OpLoc, LHSExpr, shift);
+    DiagnoseAdditionInShift(Self, OpLoc, RHSExpr, shift);
+  }
 }
 
 // Binary Operators.  'Tok' is the token for the operator.
diff --git a/clang/test/Sema/parentheses.cpp b/clang/test/Sema/parentheses.cpp
index 767416677e0..d0dcdda0bf3 100644
--- a/clang/test/Sema/parentheses.cpp
+++ b/clang/test/Sema/parentheses.cpp
@@ -22,6 +22,8 @@ public:
   operator int();
   Stream &operator<<(int);
   Stream &operator<<(const char*);
+  Stream &operator>>(int);
+  Stream &operator>>(const char*);
 };
 
 void f(Stream& s, bool b) {
@@ -45,3 +47,13 @@ void test(S *s, bool (S::*m_ptr)()) {
   // Don't crash on unusual member call expressions.
   (void)((s->*m_ptr)() ? "foo" : "bar");
 }
+
+void test(int a, int b, int c) {
+  (void)(a >> b + c); // expected-warning {{'+' within '>>'}} \
+                         expected-note {{place parentheses around the '+' expression to silence this warning}}
+  (void)(a - b << c); // expected-warning {{'-' within '<<'}} \
+                         expected-note {{place parentheses around the '-' expression to silence this warning}}
+  Stream() << b + c;
+  Stream() >> b + c; // expected-warning {{'+' within '>>'}} \
+                        expected-note {{place parentheses around the '+' expression to silence this warning}}
+}