This is an attempt to address some of http://llvm.org/bugs/show_bug.cgi?id=9969
The idea is to warn about code such as:
int foo(int x, bool someCondition) {
return x + someCondition ? 42 : 0;
}
where it seems likely that the programmer forgot that ?: has lower
precedence than +.
The patch looks for condition expressions which are non-boolean binary
operators, implicitly cast to bool, where the right-hand side
expression is bool.
Please take a look!
Thanks,
Hans
Index: test/Sema/parentheses.cpp
===================================================================
--- test/Sema/parentheses.cpp (revision 0)
+++ test/Sema/parentheses.cpp (revision 0)
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -Wparentheses -fsyntax-only -verify %s
+// RUN: %clang_cc1 -Wparentheses -fixit %s -o - | %clang_cc1 -Wparentheses -Werror -
+
+void conditional_op(int i, bool b, bool c) {
+ (void)(i + b ? 1 : 2); // 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}}
+
+ (void)(b == c ? 1 : 2); // no warning
+}
Property changes on: test/Sema/parentheses.cpp
___________________________________________________________________
Added: svn:eol-style
+ LF
Index: lib/Sema/SemaExpr.cpp
===================================================================
--- lib/Sema/SemaExpr.cpp (revision 132392)
+++ lib/Sema/SemaExpr.cpp (working copy)
@@ -6167,6 +6167,87 @@
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, ")");
+}
+
+/// 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) {
+ if (ImplicitCastExpr *CE = dyn_cast<ImplicitCastExpr>(cond)) {
+ if (BinaryOperator* OP = dyn_cast<BinaryOperator>(CE->getSubExpr())) {
+ if (OP->getType()->isBooleanType())
+ return;
+ if (ImplicitCastExpr *OPRHS = dyn_cast<ImplicitCastExpr>(OP->getRHS())) {
+ Expr *CondRHS = OPRHS->getSubExpr();
+ if (CondRHS->getType()->isBooleanType()) {
+ // Condition is a non-boolean binary operator, with boolean RHS.
+
+ 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,
@@ -6202,6 +6283,7 @@
LHSExpr = CondExpr = opaqueValue;
}
+
ExprValueKind VK = VK_RValue;
ExprObjectKind OK = OK_Ordinary;
ExprResult Cond = Owned(CondExpr), LHS = Owned(LHSExpr), RHS = Owned(RHSExpr);
@@ -6211,6 +6293,9 @@
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 +8820,6 @@
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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
