[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
Chris Lattner
clattner at apple.com
Mon Oct 12 22:24:06 PDT 2009
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
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
More information about the cfe-commits
mailing list