From 2f072b442879b8bba8c5dea11d7c61bedb1924ae Mon Sep 17 00:00:00 2001 From: Hans Wennborg Date: Thu, 9 Jun 2011 17:06:51 +0000 Subject: [PATCH] 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. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@132784 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/AST/Expr.h | 8 +++ lib/AST/Expr.cpp | 9 ++- lib/Sema/SemaExpr.cpp | 116 +++++++++++++++++++++++++------------- test/Sema/parentheses.cpp | 13 +++++ 4 files changed, 107 insertions(+), 39 deletions(-) diff --git a/include/clang/AST/Expr.h b/include/clang/AST/Expr.h index 3850c06543..ce86458ed4 100644 --- a/include/clang/AST/Expr.h +++ b/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(this)->IgnoreConversionOperator(); + } + const Expr *IgnoreParenImpCasts() const { return const_cast(this)->IgnoreParenImpCasts(); } diff --git a/lib/AST/Expr.cpp b/lib/AST/Expr.cpp index a6d9bb8760..012701d08a 100644 --- a/lib/AST/Expr.cpp +++ b/lib/AST/Expr.cpp @@ -1988,6 +1988,14 @@ Expr *Expr::IgnoreParenImpCasts() { } } +Expr *Expr::IgnoreConversionOperator() { + if (CXXMemberCallExpr *MCE = dyn_cast(this)) { + if (isa(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; } - diff --git a/lib/Sema/SemaExpr.cpp b/lib/Sema/SemaExpr.cpp index 6d8e8c6311..bb6a414f44 100644 --- a/lib/Sema/SemaExpr.cpp +++ b/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(E)) { + if (IsArithmeticOp(OP->getOpcode())) { + *Opcode = OP->getOpcode(); + *RHS = OP->getRHS(); + return true; + } + } + + // Overloaded operator. + if (CXXOperatorCallExpr *Call = dyn_cast(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(E)) + return IsLogicOp(OP->getOpcode()); + if (UnaryOperator *OP = dyn_cast(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(cond)) - cond = C->getSubExpr(); + BinaryOperatorKind CondOpcode; + Expr *CondRHS; - if (BinaryOperator *OP = dyn_cast(cond)) { - if (!IsArithmeticOp(OP->getOpcode())) - return; - - // Drill down on the RHS of the condition. - Expr *CondRHS = OP->getRHS(); - while (ImplicitCastExpr *C = dyn_cast(CondRHS)) - CondRHS = C->getSubExpr(); - while (ParenExpr *P = dyn_cast(CondRHS)) - CondRHS = P->getSubExpr(); + if (!IsArithmeticBinaryExpr(cond, &CondOpcode, &CondRHS)) + return; + if (!ExprLooksBoolean(CondRHS)) + return; - bool CondRHSLooksBoolean = false; - if (CondRHS->getType()->isBooleanType()) - CondRHSLooksBoolean = true; - else if (BinaryOperator *CondRHSOP = dyn_cast(CondRHS)) - CondRHSLooksBoolean = IsLogicOp(CondRHSOP->getOpcode()); - else if (UnaryOperator *CondRHSOP = dyn_cast(CondRHS)) - CondRHSLooksBoolean = CondRHSOP->getOpcode() == UO_LNot; + // The condition is an arithmetic binary expression, with a right- + // hand side that looks boolean, so warn. - 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) + << cond->getSourceRange() + << BinaryOperator::getOpcodeStr(CondOpcode); - 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(CondOpcode); - PartialDiagnostic FirstNote = - Self.PDiag(diag::note_precedence_conditional_silence) - << BinaryOperator::getOpcodeStr(OP->getOpcode()); + SourceRange FirstParenRange(cond->getLocStart(), + cond->getLocEnd()); - SourceRange FirstParenRange(OP->getLHS()->getLocStart(), - OP->getRHS()->getLocEnd()); + PartialDiagnostic SecondNote = + Self.PDiag(diag::note_precedence_conditional_first); - PartialDiagnostic SecondNote = - Self.PDiag(diag::note_precedence_conditional_first); - SourceRange SecondParenRange(OP->getRHS()->getLocStart(), - rhs->getLocEnd()); + SourceRange SecondParenRange(CondRHS->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 diff --git a/test/Sema/parentheses.cpp b/test/Sema/parentheses.cpp index ad1f399c64..a25f2a0ce7 100644 --- a/test/Sema/parentheses.cpp +++ b/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}} +} -- 2.40.0