Thank you very much for the comments, Chandler!
On Mon, Jun 6, 2011 at 6:55 PM, Chandler Carruth <[email protected]> wrote:
> I have a few problems with this patch.
> First, I like breaking up parts of it into simpler code, but unfortunately
> we're duplicating a fair amount of work stripping off various parts of the
> expression. I would lift the stripping logic back into the common function
> so that its only ever done once.
I'm not sure I follow you. The stripping *is* only done once:
IsArithmeticBinaryExpr works with the condition expression, and
ExprLooksBoolean works with the right-hand side of it.
Or do you mean the code is duplicated because we could just use
Expr::IgnoreParenImpCasts()? In that case I'm with you :)
> Also, doxyments would be good.
Done. I'm not too familiar with doxygen, so please nag me if I didn't
get it right.
> A few other comments on the patch itself:
> + // Remove implicit casts.
> + while (ImplicitCastExpr *C = dyn_cast<ImplicitCastExpr>(E))
> + E = C->getSubExpr();
> Do you want to leave ParenExpr's here or not? If not, use
> E->IgnoreParenImpCasts(). If so, want to add Expr::IgnoreImpCasts() and use
> that instead?
Ah, didn't know about IgnoreParenImpCasts(). Using that.
> + // Remove call to conversion op.
> + if (CXXMemberCallExpr *MCE = dyn_cast<CXXMemberCallExpr>(E)) {
> + if (isa<CXXConversionDecl>(MCE->getMethodDecl()))
> + E = MCE->getImplicitObjectArgument();
> + }
> This also seems like a useful predicate to add to Expr... Does this handle
> both coversion operators and implicit constructors? I can't remember if the
> latter are modeled as implicit casts or ConstructorExprs off the top of my
> head.
This doesn't handle implicit constructors (they're modeled as
CXXConstructExpr), and I don't think it has to either.
Since this is the condition expression, it has to be implicitly
convertable to bool, and if I remember correctly, conversions such as
"x -> implicit constructor -> conversion op" can't happen in C++.
I've added Expr::IgnoreConversionOperator().
> + if (CXXOperatorCallExpr* Call = dyn_cast<CXXOperatorCallExpr>(E)) {
> You don't ignore parens or imp casts from E which may have come from
> MCE->getImplicitObjectArgument() above.
Fixed.
> + OverloadedOperatorKind OO = Call->getOperator();
> + if (OO >= OO_Plus && OO <= OO_Arrow) {
> It would be good to comment on what this is doing. Currently it doesn't make
> a lot of sense to me, for example it includes both ! and ~. It feels like
> the num args and opcode tests below should be sufficient...
Added a comment. The purpose of the if statement is to make sure it's
safe to pass this OverloadedOperatorKind into
BinaryOperator::getOverloadedOpcode(). Not everything that takes two
arguments is a binary operator (e.g. the subscript operator).
> + if (Call->getNumArgs() == 2) {
> Throughout this, could we use early-exit to simplify the code?
Yes, doing that.
> + *Opcode = BinaryOperator::getOverloadedOpcode(OO);
> This is the only point at which we set one of the output parameters and can
> (potentially) return false... that seems a bit surprising.
Fixed.
> + if (IsArithmeticOp(*Opcode)) {
> + *RHS = Call->getArg(1);
> + return true;
>
>
Thanks for your time!
Hans
diff --git a/include/clang/AST/Expr.h b/include/clang/AST/Expr.h
index 3850c06..4b28099 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<Expr*>(this)->IgnoreConversionOperator();
+ }
+
const Expr *IgnoreParenImpCasts() const {
return const_cast<Expr*>(this)->IgnoreParenImpCasts();
}
diff --git a/lib/AST/Expr.cpp b/lib/AST/Expr.cpp
index a6d9bb8..012701d 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<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;
}
-
diff --git a/lib/Sema/SemaExpr.cpp b/lib/Sema/SemaExpr.cpp
index 1914a6a..0a67e86 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<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;
+ else if (BinaryOperator *OP = dyn_cast<BinaryOperator>(E))
+ return IsLogicOp(OP->getOpcode());
+ else 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;
-
- // 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();
+ 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<BinaryOperator>(CondRHS))
- CondRHSLooksBoolean = IsLogicOp(CondRHSOP->getOpcode());
- else if (UnaryOperator *CondRHSOP = dyn_cast<UnaryOperator>(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 ad1f399..a25f2a0 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}}
+}
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
