On Oct 12, 2009, at 2:59 PM, John McCall wrote: > 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
Awesome, thanks John. Random question: should this warning default to on? I know that GCC doesn't default it to on, but perhaps clang should? -Chris > > > 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); ) {} > +} > > > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
