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

3 files changed, 77 insertions, 48 deletions

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<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;
+}
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<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 @@ Sema::ActOnWhileStmt(SourceLocation WhileLoc, FullExprArg Cond, StmtArg Body) {
   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 @@ Sema::ActOnDoStmt(SourceLocation DoLoc, StmtArg Body,
   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 @@ Sema::ActOnForStmt(SourceLocation ForLoc, SourceLocation LParenLoc,
                    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 @@ 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,