[cfe-commits] r132565 - in /cfe/trunk: include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/SemaExpr.cpp test/Sema/parentheses.c test/Sema/parentheses.cpp
Hans Wennborg
hans at hanshq.net
Fri Jun 3 11:00:37 PDT 2011
Author: hans
Date: Fri Jun 3 13:00:36 2011
New Revision: 132565
URL: http://llvm.org/viewvc/llvm-project?rev=132565&view=rev
Log:
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
Added:
cfe/trunk/test/Sema/parentheses.cpp (with props)
Modified:
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
cfe/trunk/lib/Sema/SemaExpr.cpp
cfe/trunk/test/Sema/parentheses.c
Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=132565&r1=132564&r2=132565&view=diff
==============================================================================
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Fri Jun 3 13:00:36 2011
@@ -2562,6 +2562,14 @@
def note_precedence_bitwise_silence : Note<
"place parentheses around the %0 expression to silence this warning">;
+def warn_precedence_conditional : Warning<
+ "?: has lower precedence than %0; %0 will be evaluated first">,
+ InGroup<Parentheses>;
+def note_precedence_conditional_first : Note<
+ "place parentheses around the ?: expression to evaluate it first">;
+def note_precedence_conditional_silence : Note<
+ "place parentheses around the %0 expression to silence this warning">;
+
def warn_logical_instead_of_bitwise : Warning<
"use of logical %0 with constant operand; switch to bitwise %1 or "
"remove constant">, InGroup<DiagGroup<"constant-logical-operand">>;
Modified: cfe/trunk/lib/Sema/SemaExpr.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExpr.cpp?rev=132565&r1=132564&r2=132565&view=diff
==============================================================================
--- cfe/trunk/lib/Sema/SemaExpr.cpp (original)
+++ cfe/trunk/lib/Sema/SemaExpr.cpp Fri Jun 3 13:00:36 2011
@@ -6167,6 +6167,110 @@
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 @@
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 @@
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
Modified: cfe/trunk/test/Sema/parentheses.c
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/parentheses.c?rev=132565&r1=132564&r2=132565&view=diff
==============================================================================
--- cfe/trunk/test/Sema/parentheses.c (original)
+++ cfe/trunk/test/Sema/parentheses.c Fri Jun 3 13:00:36 2011
@@ -39,6 +39,28 @@
(void)(0 || i && i); // no warning.
}
+_Bool someConditionFunc();
+
+void conditional_op(int x, int y, _Bool b) {
+ (void)(x + someConditionFunc() ? 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)(x - 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)(x * (x == y) ? 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)(x / !x ? 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)(x % 2 ? 1 : 2); // no warning
+}
+
// RUN: %clang_cc1 -fsyntax-only -Wparentheses -Werror -fdiagnostics-show-option %s 2>&1 | FileCheck %s
// CHECK: error: using the result of an assignment as a condition without parentheses [-Werror,-Wparentheses]
-
Added: cfe/trunk/test/Sema/parentheses.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/parentheses.cpp?rev=132565&view=auto
==============================================================================
--- cfe/trunk/test/Sema/parentheses.cpp (added)
+++ cfe/trunk/test/Sema/parentheses.cpp Fri Jun 3 13:00:36 2011
@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 -Wparentheses -fsyntax-only -verify %s
+// RUN: %clang_cc1 -Wparentheses -fixit %s -o - | %clang_cc1 -Wparentheses -Werror -
+
+bool someConditionFunc();
+
+void conditional_op(int x, int y, bool b) {
+ (void)(x + someConditionFunc() ? 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)(x - 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)(x * (x == y) ? 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}}
+}
Propchange: cfe/trunk/test/Sema/parentheses.cpp
------------------------------------------------------------------------------
svn:eol-style = LF
More information about the cfe-commits
mailing list