Warn about missing parentheses for conditional operator.

Warn in cases such as "x + someCondition ? 42 : 0;",
where the condition expression looks arithmetic, and has
a right-hand side that looks boolean.

This (partly) addresses http://llvm.org/bugs/show_bug.cgi?id=9969

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@132565 91177308-0d34-0410-b5e6-96231b3b80d8

1 files changed, 107 insertions, 39 deletions

diff --git a/lib/Sema/SemaExpr.cpp b/lib/Sema/SemaExpr.cpp
index 9afd5f8ccd..1006b15ff1 100644
--- a/lib/Sema/SemaExpr.cpp
+++ b/lib/Sema/SemaExpr.cpp
@@ -6167,6 +6167,110 @@ QualType Sema::FindCompositeObjCPointerType(ExprResult &LHS, ExprResult &RHS,
   return QualType();
 }
 
+/// SuggestParentheses - Emit a diagnostic together with a fixit hint that wraps
+/// ParenRange in parentheses.
+static void SuggestParentheses(Sema &Self, SourceLocation Loc,
+                               const PartialDiagnostic &PD,
+                               const PartialDiagnostic &FirstNote,
+                               SourceRange FirstParenRange,
+                               const PartialDiagnostic &SecondNote,
+                               SourceRange SecondParenRange) {
+  Self.Diag(Loc, PD);
+
+  if (!FirstNote.getDiagID())
+    return;
+
+  SourceLocation EndLoc = Self.PP.getLocForEndOfToken(FirstParenRange.getEnd());
+  if (!FirstParenRange.getEnd().isFileID() || EndLoc.isInvalid()) {
+    // We can't display the parentheses, so just return.
+    return;
+  }
+
+  Self.Diag(Loc, FirstNote)
+    << FixItHint::CreateInsertion(FirstParenRange.getBegin(), "(")
+    << FixItHint::CreateInsertion(EndLoc, ")");
+
+  if (!SecondNote.getDiagID())
+    return;
+
+  EndLoc = Self.PP.getLocForEndOfToken(SecondParenRange.getEnd());
+  if (!SecondParenRange.getEnd().isFileID() || EndLoc.isInvalid()) {
+    // We can't display the parentheses, so just dig the
+    // warning/error and return.
+    Self.Diag(Loc, SecondNote);
+    return;
+  }
+
+  Self.Diag(Loc, SecondNote)
+    << FixItHint::CreateInsertion(SecondParenRange.getBegin(), "(")
+    << FixItHint::CreateInsertion(EndLoc, ")");
+}
+
+static bool IsArithmeticOp(BinaryOperatorKind Opc) {
+  return Opc >= BO_Mul && Opc <= BO_Shr;
+}
+
+static bool IsLogicOp(BinaryOperatorKind Opc) {
+  return (Opc >= BO_LT && Opc <= BO_NE) || (Opc >= BO_LAnd && Opc <= BO_LOr);
+}
+
+/// DiagnoseConditionalPrecedence - Emit a warning when a conditional operator
+/// and binary operator are mixed in a way that suggests the programmer assumed
+/// the conditional operator has higher precedence, for example:
+/// "int x = a + someBinaryCondition ? 1 : 2".
+static void DiagnoseConditionalPrecedence(Sema &Self,
+                                          SourceLocation OpLoc,
+                                          Expr *cond,
+                                          Expr *lhs,
+                                          Expr *rhs) {
+  while (ImplicitCastExpr *C = dyn_cast<ImplicitCastExpr>(cond))
+    cond = C->getSubExpr();
+
+  if (BinaryOperator *OP = dyn_cast<BinaryOperator>(cond)) {
+    if (!IsArithmeticOp(OP->getOpcode()))
+      return;
+
+    // Drill down on the RHS of the condition.
+    Expr *CondRHS = OP->getRHS();
+    while (ImplicitCastExpr *C = dyn_cast<ImplicitCastExpr>(CondRHS))
+      CondRHS = C->getSubExpr();
+    while (ParenExpr *P = dyn_cast<ParenExpr>(CondRHS))
+      CondRHS = P->getSubExpr();
+
+    bool CondRHSLooksBoolean = false;
+    if (CondRHS->getType()->isBooleanType())
+      CondRHSLooksBoolean = true;
+    else if (BinaryOperator *CondRHSOP = dyn_cast<BinaryOperator>(CondRHS))
+      CondRHSLooksBoolean = IsLogicOp(CondRHSOP->getOpcode());
+    else if (UnaryOperator *CondRHSOP = dyn_cast<UnaryOperator>(CondRHS))
+      CondRHSLooksBoolean = CondRHSOP->getOpcode() == UO_LNot;
+
+    if (CondRHSLooksBoolean) {
+      // The condition is an arithmetic binary expression, with a right-
+      // hand side that looks boolean, so warn.
+
+      PartialDiagnostic Warn = Self.PDiag(diag::warn_precedence_conditional)
+          << OP->getSourceRange()
+          << BinaryOperator::getOpcodeStr(OP->getOpcode());
+
+      PartialDiagnostic FirstNote =
+          Self.PDiag(diag::note_precedence_conditional_silence)
+          << BinaryOperator::getOpcodeStr(OP->getOpcode());
+
+      SourceRange FirstParenRange(OP->getLHS()->getLocStart(),
+                                  OP->getRHS()->getLocEnd());
+
+      PartialDiagnostic SecondNote =
+          Self.PDiag(diag::note_precedence_conditional_first);
+      SourceRange SecondParenRange(OP->getRHS()->getLocStart(),
+                                   rhs->getLocEnd());
+
+      SuggestParentheses(Self, OpLoc, Warn, FirstNote, FirstParenRange,
+                         SecondNote, SecondParenRange);
+    }
+  }
+}
+
 /// ActOnConditionalOp - Parse a ?: operation.  Note that 'LHS' may be null
 /// in the case of a the GNU conditional expr extension.
 ExprResult Sema::ActOnConditionalOp(SourceLocation QuestionLoc,
@@ -6211,6 +6315,9 @@ ExprResult Sema::ActOnConditionalOp(SourceLocation QuestionLoc,
       RHS.isInvalid())
     return ExprError();
 
+  DiagnoseConditionalPrecedence(*this, QuestionLoc, Cond.get(), LHS.get(),
+                                RHS.get());
+
   if (!commonExpr)
     return Owned(new (Context) ConditionalOperator(Cond.take(), QuestionLoc,
                                                    LHS.take(), ColonLoc, 
@@ -8735,45 +8842,6 @@ ExprResult Sema::CreateBuiltinBinOp(SourceLocation OpLoc,
                                                     CompResultTy, OpLoc));
 }
 
-/// SuggestParentheses - Emit a diagnostic together with a fixit hint that wraps
-/// ParenRange in parentheses.
-static void SuggestParentheses(Sema &Self, SourceLocation Loc,
-                               const PartialDiagnostic &PD,
-                               const PartialDiagnostic &FirstNote,
-                               SourceRange FirstParenRange,
-                               const PartialDiagnostic &SecondNote,
-                               SourceRange SecondParenRange) {
-  Self.Diag(Loc, PD);
-
-  if (!FirstNote.getDiagID())
-    return;
-
-  SourceLocation EndLoc = Self.PP.getLocForEndOfToken(FirstParenRange.getEnd());
-  if (!FirstParenRange.getEnd().isFileID() || EndLoc.isInvalid()) {
-    // We can't display the parentheses, so just return.
-    return;
-  }
-
-  Self.Diag(Loc, FirstNote)
-    << FixItHint::CreateInsertion(FirstParenRange.getBegin(), "(")
-    << FixItHint::CreateInsertion(EndLoc, ")");
-
-  if (!SecondNote.getDiagID())
-    return;
-
-  EndLoc = Self.PP.getLocForEndOfToken(SecondParenRange.getEnd());
-  if (!SecondParenRange.getEnd().isFileID() || EndLoc.isInvalid()) {
-    // We can't display the parentheses, so just dig the
-    // warning/error and return.
-    Self.Diag(Loc, SecondNote);
-    return;
-  }
-
-  Self.Diag(Loc, SecondNote)
-    << FixItHint::CreateInsertion(SecondParenRange.getBegin(), "(")
-    << FixItHint::CreateInsertion(EndLoc, ")");
-}
-
 /// DiagnoseBitwisePrecedence - Emit a warning when bitwise and comparison
 /// operators are mixed in a way that suggests that the programmer forgot that
 /// comparison operators have higher precedence. The most typical example of