From: John McCall Date: Mon, 12 Oct 2009 21:59:07 +0000 (+0000) Subject: Implement -Wparentheses: warn about using assignments in contexts that require X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=5a881bb09928b7ade891efc680088aaad276f8d6;p=clang 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 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@83907 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td index cdc121f342..557615bee5 100644 --- a/include/clang/Basic/DiagnosticSemaKinds.td +++ b/include/clang/Basic/DiagnosticSemaKinds.td @@ -1727,7 +1727,11 @@ def err_not_tag_in_scope : Error< 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, 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">; diff --git a/lib/Sema/Sema.h b/lib/Sema/Sema.h index 7b6fa47e28..40e7426e97 100644 --- a/lib/Sema/Sema.h +++ b/lib/Sema/Sema.h @@ -3675,6 +3675,20 @@ public: 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); diff --git a/lib/Sema/SemaExpr.cpp b/lib/Sema/SemaExpr.cpp index 3dc7d8f4ef..7c1c62c0a0 100644 --- a/lib/Sema/SemaExpr.cpp +++ b/lib/Sema/SemaExpr.cpp @@ -6268,3 +6268,56 @@ bool Sema::CheckCallReturnType(QualType ReturnType, SourceLocation Loc, 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(E)) { + BinaryOperator *Op = cast(E); + if (Op->getOpcode() != BinaryOperator::Assign) + return; + + Loc = Op->getOperatorLoc(); + } else if (isa(E)) { + CXXOperatorCallExpr *Op = cast(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; +} diff --git a/lib/Sema/SemaStmt.cpp b/lib/Sema/SemaStmt.cpp index 5dd56b2221..2a3b9eeea8 100644 --- a/lib/Sema/SemaStmt.cpp +++ b/lib/Sema/SemaStmt.cpp @@ -214,20 +214,9 @@ Sema::ActOnIfStmt(SourceLocation IfLoc, FullExprArg CondVal, Expr *condExpr = CondResult.takeAs(); 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(); @@ -577,18 +566,9 @@ Sema::ActOnWhileStmt(SourceLocation WhileLoc, FullExprArg Cond, StmtArg Body) { Expr *condExpr = CondArg.takeAs(); 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(); @@ -605,18 +585,9 @@ Sema::ActOnDoStmt(SourceLocation DoLoc, StmtArg Body, Expr *condExpr = Cond.takeAs(); 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(); @@ -632,7 +603,7 @@ Sema::ActOnForStmt(SourceLocation ForLoc, SourceLocation LParenLoc, StmtArg first, ExprArg second, ExprArg third, SourceLocation RParenLoc, StmtArg body) { Stmt *First = static_cast(first.get()); - Expr *Second = static_cast(second.get()); + Expr *Second = second.takeAs(); Expr *Third = static_cast(third.get()); Stmt *Body = static_cast(body.get()); @@ -652,17 +623,9 @@ Sema::ActOnForStmt(SourceLocation ForLoc, SourceLocation LParenLoc, } } } - 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 @@ Sema::ActOnForStmt(SourceLocation ForLoc, SourceLocation LParenLoc, DiagnoseUnusedExprResult(Body); first.release(); - second.release(); third.release(); body.release(); return Owned(new (Context) ForStmt(First, Second, Third, Body, ForLoc, diff --git a/test/SemaCXX/warn-assignment-condition.cpp b/test/SemaCXX/warn-assignment-condition.cpp new file mode 100644 index 0000000000..3b9f3066a1 --- /dev/null +++ b/test/SemaCXX/warn-assignment-condition.cpp @@ -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); ) {} +}