[cfe-commits] r83907 - in /cfe/trunk: include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/Sema.h lib/Sema/SemaExpr.cpp lib/Sema/SemaStmt.cpp test/SemaCXX/warn-assignment-condition.cpp
John McCall
rjmccall at apple.com
Mon Oct 12 14:59:07 PDT 2009
Author: rjmccall
Date: Mon Oct 12 16:59:07 2009
New Revision: 83907
URL: http://llvm.org/viewvc/llvm-project?rev=83907&view=rev
Log:
Implement -Wparentheses: warn about using assignments in contexts that require
conditions. Add a fixit to insert the parentheses. Also fix a very minor
possible memory leak in 'for' conditions.
Fixes PR 4876 and rdar://problem/7289172
Added:
cfe/trunk/test/SemaCXX/warn-assignment-condition.cpp
Modified:
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
cfe/trunk/lib/Sema/Sema.h
cfe/trunk/lib/Sema/SemaExpr.cpp
cfe/trunk/lib/Sema/SemaStmt.cpp
Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=83907&r1=83906&r2=83907&view=diff
==============================================================================
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Mon Oct 12 16:59:07 2009
@@ -1727,7 +1727,11 @@
def err_cannot_form_pointer_to_member_of_reference_type : Error<
"cannot form a pointer-to-member to member %0 of reference type %1">;
-
+
+def warn_condition_is_assignment : Warning<"using the result of an "
+ "assignment as a condition without parentheses">,
+ InGroup<Parentheses>, DefaultIgnore;
+
def warn_value_always_zero : Warning<"%0 is always zero in this context">;
def warn_value_always_false : Warning<"%0 is always false in this context">;
Modified: cfe/trunk/lib/Sema/Sema.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/Sema.h?rev=83907&r1=83906&r2=83907&view=diff
==============================================================================
--- cfe/trunk/lib/Sema/Sema.h (original)
+++ cfe/trunk/lib/Sema/Sema.h Mon Oct 12 16:59:07 2009
@@ -3675,6 +3675,20 @@
SourceLocation lbrac, SourceLocation rbrac,
QualType &ReturnType);
+ /// CheckBooleanCondition - Diagnose problems involving the use of
+ /// the given expression as a boolean condition (e.g. in an if
+ /// statement). Also performs the standard function and array
+ /// decays, possibly changing the input variable.
+ ///
+ /// \param Loc - A location associated with the condition, e.g. the
+ /// 'if' keyword.
+ /// \return true iff there were any errors
+ bool CheckBooleanCondition(Expr *&CondExpr, SourceLocation Loc);
+
+ /// DiagnoseAssignmentAsCondition - Given that an expression is
+ /// being used as a boolean condition, warn if it's an assignment.
+ void DiagnoseAssignmentAsCondition(Expr *E);
+
/// CheckCXXBooleanCondition - Returns true if conversion to bool is invalid.
bool CheckCXXBooleanCondition(Expr *&CondExpr);
Modified: cfe/trunk/lib/Sema/SemaExpr.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExpr.cpp?rev=83907&r1=83906&r2=83907&view=diff
==============================================================================
--- cfe/trunk/lib/Sema/SemaExpr.cpp (original)
+++ cfe/trunk/lib/Sema/SemaExpr.cpp Mon Oct 12 16:59:07 2009
@@ -6268,3 +6268,56 @@
return false;
}
+// Diagnose the common s/=/==/ typo. Note that adding parentheses
+// will prevent this condition from triggering, which is what we want.
+void Sema::DiagnoseAssignmentAsCondition(Expr *E) {
+ SourceLocation Loc;
+
+ if (isa<BinaryOperator>(E)) {
+ BinaryOperator *Op = cast<BinaryOperator>(E);
+ if (Op->getOpcode() != BinaryOperator::Assign)
+ return;
+
+ Loc = Op->getOperatorLoc();
+ } else if (isa<CXXOperatorCallExpr>(E)) {
+ CXXOperatorCallExpr *Op = cast<CXXOperatorCallExpr>(E);
+ if (Op->getOperator() != OO_Equal)
+ return;
+
+ Loc = Op->getOperatorLoc();
+ } else {
+ // Not an assignment.
+ return;
+ }
+
+ // We want to insert before the start of the expression...
+ SourceLocation Open = E->getSourceRange().getBegin();
+ // ...and one character after the end.
+ SourceLocation Close = E->getSourceRange().getEnd().getFileLocWithOffset(1);
+
+ Diag(Loc, diag::warn_condition_is_assignment)
+ << E->getSourceRange()
+ << CodeModificationHint::CreateInsertion(Open, "(")
+ << CodeModificationHint::CreateInsertion(Close, ")");
+}
+
+bool Sema::CheckBooleanCondition(Expr *&E, SourceLocation Loc) {
+ DiagnoseAssignmentAsCondition(E);
+
+ if (!E->isTypeDependent()) {
+ DefaultFunctionArrayConversion(E);
+
+ QualType T = E->getType();
+
+ if (getLangOptions().CPlusPlus) {
+ if (CheckCXXBooleanCondition(E)) // C++ 6.4p4
+ return true;
+ } else if (!T->isScalarType()) { // C99 6.8.4.1p1
+ Diag(Loc, diag::err_typecheck_statement_requires_scalar)
+ << T << E->getSourceRange();
+ return true;
+ }
+ }
+
+ return false;
+}
Modified: cfe/trunk/lib/Sema/SemaStmt.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaStmt.cpp?rev=83907&r1=83906&r2=83907&view=diff
==============================================================================
--- cfe/trunk/lib/Sema/SemaStmt.cpp (original)
+++ cfe/trunk/lib/Sema/SemaStmt.cpp Mon Oct 12 16:59:07 2009
@@ -214,20 +214,9 @@
Expr *condExpr = CondResult.takeAs<Expr>();
assert(condExpr && "ActOnIfStmt(): missing expression");
-
- if (!condExpr->isTypeDependent()) {
- DefaultFunctionArrayConversion(condExpr);
- // Take ownership again until we're past the error checking.
+ if (CheckBooleanCondition(condExpr, IfLoc)) {
CondResult = condExpr;
- QualType condType = condExpr->getType();
-
- if (getLangOptions().CPlusPlus) {
- if (CheckCXXBooleanCondition(condExpr)) // C++ 6.4p4
- return StmtError();
- } else if (!condType->isScalarType()) // C99 6.8.4.1p1
- return StmtError(Diag(IfLoc,
- diag::err_typecheck_statement_requires_scalar)
- << condType << condExpr->getSourceRange());
+ return StmtError();
}
Stmt *thenStmt = ThenVal.takeAs<Stmt>();
@@ -577,18 +566,9 @@
Expr *condExpr = CondArg.takeAs<Expr>();
assert(condExpr && "ActOnWhileStmt(): missing expression");
- if (!condExpr->isTypeDependent()) {
- DefaultFunctionArrayConversion(condExpr);
+ if (CheckBooleanCondition(condExpr, WhileLoc)) {
CondArg = condExpr;
- QualType condType = condExpr->getType();
-
- if (getLangOptions().CPlusPlus) {
- if (CheckCXXBooleanCondition(condExpr)) // C++ 6.4p4
- return StmtError();
- } else if (!condType->isScalarType()) // C99 6.8.5p2
- return StmtError(Diag(WhileLoc,
- diag::err_typecheck_statement_requires_scalar)
- << condType << condExpr->getSourceRange());
+ return StmtError();
}
Stmt *bodyStmt = Body.takeAs<Stmt>();
@@ -605,18 +585,9 @@
Expr *condExpr = Cond.takeAs<Expr>();
assert(condExpr && "ActOnDoStmt(): missing expression");
- if (!condExpr->isTypeDependent()) {
- DefaultFunctionArrayConversion(condExpr);
+ if (CheckBooleanCondition(condExpr, DoLoc)) {
Cond = condExpr;
- QualType condType = condExpr->getType();
-
- if (getLangOptions().CPlusPlus) {
- if (CheckCXXBooleanCondition(condExpr)) // C++ 6.4p4
- return StmtError();
- } else if (!condType->isScalarType()) // C99 6.8.5p2
- return StmtError(Diag(DoLoc,
- diag::err_typecheck_statement_requires_scalar)
- << condType << condExpr->getSourceRange());
+ return StmtError();
}
Stmt *bodyStmt = Body.takeAs<Stmt>();
@@ -632,7 +603,7 @@
StmtArg first, ExprArg second, ExprArg third,
SourceLocation RParenLoc, StmtArg body) {
Stmt *First = static_cast<Stmt*>(first.get());
- Expr *Second = static_cast<Expr*>(second.get());
+ Expr *Second = second.takeAs<Expr>();
Expr *Third = static_cast<Expr*>(third.get());
Stmt *Body = static_cast<Stmt*>(body.get());
@@ -652,17 +623,9 @@
}
}
}
- if (Second && !Second->isTypeDependent()) {
- DefaultFunctionArrayConversion(Second);
- QualType SecondType = Second->getType();
-
- if (getLangOptions().CPlusPlus) {
- if (CheckCXXBooleanCondition(Second)) // C++ 6.4p4
- return StmtError();
- } else if (!SecondType->isScalarType()) // C99 6.8.5p2
- return StmtError(Diag(ForLoc,
- diag::err_typecheck_statement_requires_scalar)
- << SecondType << Second->getSourceRange());
+ if (Second && CheckBooleanCondition(Second, ForLoc)) {
+ second = Second;
+ return StmtError();
}
DiagnoseUnusedExprResult(First);
@@ -670,7 +633,6 @@
DiagnoseUnusedExprResult(Body);
first.release();
- second.release();
third.release();
body.release();
return Owned(new (Context) ForStmt(First, Second, Third, Body, ForLoc,
Added: cfe/trunk/test/SemaCXX/warn-assignment-condition.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-assignment-condition.cpp?rev=83907&view=auto
==============================================================================
--- cfe/trunk/test/SemaCXX/warn-assignment-condition.cpp (added)
+++ cfe/trunk/test/SemaCXX/warn-assignment-condition.cpp Mon Oct 12 16:59:07 2009
@@ -0,0 +1,65 @@
+// RUN: clang-cc -fsyntax-only -Wparentheses -verify %s
+
+struct A {
+ int foo();
+ friend A operator+(const A&, const A&);
+ operator bool();
+};
+
+void test() {
+ int x, *p;
+ A a, b;
+
+ // With scalars.
+ if (x = 7) {} // expected-warning {{using the result of an assignment as a condition without parentheses}}
+ if ((x = 7)) {}
+ do {
+ } while (x = 7); // expected-warning {{using the result of an assignment as a condition without parentheses}}
+ do {
+ } while ((x = 7));
+ while (x = 7) {} // expected-warning {{using the result of an assignment as a condition without parentheses}}
+ while ((x = 7)) {}
+ for (; x = 7; ) {} // expected-warning {{using the result of an assignment as a condition without parentheses}}
+ for (; (x = 7); ) {}
+
+ if (p = p) {} // expected-warning {{using the result of an assignment as a condition without parentheses}}
+ if ((p = p)) {}
+ do {
+ } while (p = p); // expected-warning {{using the result of an assignment as a condition without parentheses}}
+ do {
+ } while ((p = p));
+ while (p = p) {} // expected-warning {{using the result of an assignment as a condition without parentheses}}
+ while ((p = p)) {}
+ for (; p = p; ) {} // expected-warning {{using the result of an assignment as a condition without parentheses}}
+ for (; (p = p); ) {}
+
+ // Initializing variables (shouldn't warn).
+ if (int y = x) {}
+ while (int y = x) {}
+ if (A y = a) {}
+ while (A y = a) {}
+
+ // With temporaries.
+ if (x = (b+b).foo()) {} // expected-warning {{using the result of an assignment as a condition without parentheses}}
+ if ((x = (b+b).foo())) {}
+ do {
+ } while (x = (b+b).foo()); // expected-warning {{using the result of an assignment as a condition without parentheses}}
+ do {
+ } while ((x = (b+b).foo()));
+ while (x = (b+b).foo()) {} // expected-warning {{using the result of an assignment as a condition without parentheses}}
+ while ((x = (b+b).foo())) {}
+ for (; x = (b+b).foo(); ) {} // expected-warning {{using the result of an assignment as a condition without parentheses}}
+ for (; (x = (b+b).foo()); ) {}
+
+ // With a user-defined operator.
+ if (a = b + b) {} // expected-warning {{using the result of an assignment as a condition without parentheses}}
+ if ((a = b + b)) {}
+ do {
+ } while (a = b + b); // expected-warning {{using the result of an assignment as a condition without parentheses}}
+ do {
+ } while ((a = b + b));
+ while (a = b + b) {} // expected-warning {{using the result of an assignment as a condition without parentheses}}
+ while ((a = b + b)) {}
+ for (; a = b + b; ) {} // expected-warning {{using the result of an assignment as a condition without parentheses}}
+ for (; (a = b + b); ) {}
+}
More information about the cfe-commits
mailing list