Handle overloaded operators in ?: precedence warning

This is a follow-up to r132565, and should address the rest of PR9969:

Warn about cases such as

int foo(A a, bool b) {
 return a + b ? 1 : 2; // user probably meant a + (b ? 1 : 2);
}

also when + is an overloaded operator call.

llvm-svn: 132784

clang/include/clang/AST/Expr.h

@@ -515,6 +515,14 @@ public:
   /// ParenExpr or ImplicitCastExprs, returning their operand.
   Expr *IgnoreParenImpCasts();
 
+  /// IgnoreConversionOperator - Ignore conversion operator. If this Expr is a
+  /// call to a conversion operator, return the argument.
+  Expr *IgnoreConversionOperator();
+
+  const Expr *IgnoreConversionOperator() const {
+    return const_cast<Expr*>(this)->IgnoreConversionOperator();
+  }
+
   const Expr *IgnoreParenImpCasts() const {
     return const_cast<Expr*>(this)->IgnoreParenImpCasts();
   }

clang/lib/AST/Expr.cpp

@@ -1988,6 +1988,14 @@ Expr *Expr::IgnoreParenImpCasts() {
   }
 }
 
+Expr *Expr::IgnoreConversionOperator() {
+  if (CXXMemberCallExpr *MCE = dyn_cast<CXXMemberCallExpr>(this)) {
+    if (isa<CXXConversionDecl>(MCE->getMethodDecl()))
+      return MCE->getImplicitObjectArgument();
+  }
+  return this;
+}
+
 /// IgnoreParenNoopCasts - Ignore parentheses and casts that do not change the
 /// value (including ptr->int casts of the same size).  Strip off any
 /// ParenExpr or CastExprs, returning their operand.
@@ -3023,4 +3031,3 @@ BlockDeclRefExpr::BlockDeclRefExpr(VarDecl *d, QualType t, ExprValueKind VK,
   ExprBits.TypeDependent = TypeDependent;
   ExprBits.ValueDependent = ValueDependent;
 }
-

clang/lib/Sema/SemaExpr.cpp

@@ -6230,10 +6230,66 @@ static bool IsArithmeticOp(BinaryOperatorKind Opc) {
   return Opc >= BO_Mul && Opc <= BO_Shr;
 }
 
+/// IsArithmeticBinaryExpr - Returns true if E is an arithmetic binary
+/// expression, either using a built-in or overloaded operator,
+/// and sets *OpCode to the opcode and *RHS to the right-hand side expression.
+static bool IsArithmeticBinaryExpr(Expr *E, BinaryOperatorKind *Opcode,
+                                   Expr **RHS) {
+  E = E->IgnoreParenImpCasts();
+  E = E->IgnoreConversionOperator();
+  E = E->IgnoreParenImpCasts();
+
+  // Built-in binary operator.
+  if (BinaryOperator *OP = dyn_cast<BinaryOperator>(E)) {
+    if (IsArithmeticOp(OP->getOpcode())) {
+      *Opcode = OP->getOpcode();
+      *RHS = OP->getRHS();
+      return true;
+    }
+  }
+
+  // Overloaded operator.
+  if (CXXOperatorCallExpr *Call = dyn_cast<CXXOperatorCallExpr>(E)) {
+    if (Call->getNumArgs() != 2)
+      return false;
+
+    // Make sure this is really a binary operator that is safe to pass into
+    // BinaryOperator::getOverloadedOpcode(), e.g. it's not a subscript op.
+    OverloadedOperatorKind OO = Call->getOperator();
+    if (OO < OO_Plus || OO > OO_Arrow)
+      return false;
+
+    BinaryOperatorKind OpKind = BinaryOperator::getOverloadedOpcode(OO);
+    if (IsArithmeticOp(OpKind)) {
+      *Opcode = OpKind;
+      *RHS = Call->getArg(1);
+      return true;
+    }
+  }
+
+  return false;
+}
+
 static bool IsLogicOp(BinaryOperatorKind Opc) {
   return (Opc >= BO_LT && Opc <= BO_NE) || (Opc >= BO_LAnd && Opc <= BO_LOr);
 }
 
+/// ExprLooksBoolean - Returns true if E looks boolean, i.e. it has boolean type
+/// or is a logical expression such as (x==y) which has int type, but is
+/// commonly interpreted as boolean.
+static bool ExprLooksBoolean(Expr *E) {
+  E = E->IgnoreParenImpCasts();
+
+  if (E->getType()->isBooleanType())
+    return true;
+  if (BinaryOperator *OP = dyn_cast<BinaryOperator>(E))
+    return IsLogicOp(OP->getOpcode());
+  if (UnaryOperator *OP = dyn_cast<UnaryOperator>(E))
+    return OP->getOpcode() == UO_LNot;
+
+  return false;
+}
+
 /// 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:
@@ -6243,52 +6299,36 @@ static void DiagnoseConditionalPrecedence(Sema &Self,
                                           Expr *cond,
                                           Expr *lhs,
                                           Expr *rhs) {
-  while (ImplicitCastExpr *C = dyn_cast<ImplicitCastExpr>(cond))
-    cond = C->getSubExpr();
+  BinaryOperatorKind CondOpcode;
+  Expr *CondRHS;
 
-  if (BinaryOperator *OP = dyn_cast<BinaryOperator>(cond)) {
-    if (!IsArithmeticOp(OP->getOpcode()))
-      return;
+  if (!IsArithmeticBinaryExpr(cond, &CondOpcode, &CondRHS))
+    return;
+  if (!ExprLooksBoolean(CondRHS))
+    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();
+  // The condition is an arithmetic binary expression, with a right-
+  // hand side that looks boolean, so warn.
 
-    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;
+  PartialDiagnostic Warn = Self.PDiag(diag::warn_precedence_conditional)
+      << cond->getSourceRange()
+      << BinaryOperator::getOpcodeStr(CondOpcode);
 
-    if (CondRHSLooksBoolean) {
-      // The condition is an arithmetic binary expression, with a right-
-      // hand side that looks boolean, so warn.
+  PartialDiagnostic FirstNote =
+      Self.PDiag(diag::note_precedence_conditional_silence)
+      << BinaryOperator::getOpcodeStr(CondOpcode);
 
-      PartialDiagnostic Warn = Self.PDiag(diag::warn_precedence_conditional)
-          << OP->getSourceRange()
-          << BinaryOperator::getOpcodeStr(OP->getOpcode());
+  SourceRange FirstParenRange(cond->getLocStart(),
+                              cond->getLocEnd());
 
-      PartialDiagnostic FirstNote =
-          Self.PDiag(diag::note_precedence_conditional_silence)
-          << BinaryOperator::getOpcodeStr(OP->getOpcode());
+  PartialDiagnostic SecondNote =
+      Self.PDiag(diag::note_precedence_conditional_first);
 
-      SourceRange FirstParenRange(OP->getLHS()->getLocStart(),
-                                  OP->getRHS()->getLocEnd());
+  SourceRange SecondParenRange(CondRHS->getLocStart(),
+                               rhs->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);
-    }
-  }
+  SuggestParentheses(Self, OpLoc, Warn, FirstNote, FirstParenRange,
+                     SecondNote, SecondParenRange);
 }
 
 /// ActOnConditionalOp - Parse a ?: operation.  Note that 'LHS' may be null

clang/test/Sema/parentheses.cpp

@@ -16,3 +16,16 @@ void conditional_op(int x, int y, bool b) {
                                 // expected-note {{place parentheses around the ?: expression to evaluate it first}} \
                                 // expected-note {{place parentheses around the * expression to silence this warning}}
 }
+
+class Stream {
+public:
+  operator int();
+  Stream &operator<<(int);
+  Stream &operator<<(const char*);
+};
+
+void f(Stream& s, bool b) {
+  (void)(s << b ? "foo" : "bar"); // expected-warning {{?: has lower precedence than <<}} \
+                                  // expected-note {{place parentheses around the ?: expression to evaluate it first}} \
+                                  // expected-note {{place parentheses around the << expression to silence this warning}}
+}