On Wed, Jun 1, 2011 at 11:31 PM, Douglas Gregor <[email protected]> wrote:
> +/// 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;
>
> This isn't going to work in C, because comparison operators in C have type
> 'int'. Rather than looking at the type of the BinaryOperator, why not look
> for specific operators (+, -, etc.) that are likely to be misunderstood?
I tried just looking for arithmetic operators (+,-,*,/,%), but then we
warn on code such as:
TextDirection direction() const { return m_bidiEmbeddingLevel % 2
? RTL : LTR; }
I suppose we could just exclude the % operator, but then we miss out
on warning for stuff like
int x = y % (foo==bar) ? 16 : 32;
So I think the trick to getting this warning tight is to try and
figure out if the right-hand side of the condition expression looks
boolean.
Attaching a new patch that tries to do this.
>
> Also, it looks like your patch is missing the required changes to
> DiagnosticSemaKinds.td.
Oops, adding that.
>
> Finally, did you try running this patch over (say) the LLVM + Clang code
> base, to evaluate its signal/noise ratio?
This warning doesn't find anything in LLVM/Clang. In Chromium it finds
this, which looks like a bug :)
gmt = non_gmt + (zone[0] == '+') ? offset : -offset;
Thanks for your input,
Hans
Index: test/Sema/parentheses.cpp
===================================================================
--- test/Sema/parentheses.cpp (revision 0)
+++ test/Sema/parentheses.cpp (revision 0)
@@ -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}}
+}
Property changes on: test/Sema/parentheses.cpp
___________________________________________________________________
Added: svn:eol-style
+ LF
Index: test/Sema/parentheses.c
===================================================================
--- test/Sema/parentheses.c (revision 132456)
+++ test/Sema/parentheses.c (working copy)
@@ -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]
-
Index: include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- include/clang/Basic/DiagnosticSemaKinds.td (revision 132456)
+++ include/clang/Basic/DiagnosticSemaKinds.td (working copy)
@@ -2565,6 +2565,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">>;
Index: lib/Sema/SemaExpr.cpp
===================================================================
--- lib/Sema/SemaExpr.cpp (revision 132456)
+++ lib/Sema/SemaExpr.cpp (working copy)
@@ -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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
