[PATCH] D63089: [clang] Warn on implicit boolean casts in more contexts (PR34180)

[Mateusz Maćkowski via Phabricator via cfe-commits]

m4tx created this revision.
m4tx added reviewers: lebedev.ri, rsmith, klimek.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

  int x;
  return x = 5;

For a code like above, GCC produces a warning suggesting using parentheses about the assignment. This change makes clang produce similar warning to, suggesting either changing the operator to `==`, or wrap the expression inside parentheses.

This is based off D45401 <https://reviews.llvm.org/D45401>.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D63089

Files:
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/test/Sema/parentheses.c
  clang/test/Sema/parentheses.cpp


Index: clang/test/Sema/parentheses.cpp
===================================================================
--- clang/test/Sema/parentheses.cpp
+++ clang/test/Sema/parentheses.cpp
@@ -215,3 +215,15 @@
     // fix-it:"{{.*}}":{[[@LINE-9]]:20-[[@LINE-9]]:20}:")"
   }
 }
+
+bool return_assign() {
+  int i;
+  return i = 4; // expected-warning {{assignment as a condition}} \
+                // expected-note{{place parentheses around the assignment to silence this warning}} \
+                // expected-note{{use '==' to turn this assignment into an equality comparison}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:10-[[@LINE-3]]:10}:"("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:15-[[@LINE-4]]:15}:")"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-5]]:12-[[@LINE-5]]:13}:"=="
+
+  return (i = 4);
+}
Index: clang/test/Sema/parentheses.c
===================================================================
--- clang/test/Sema/parentheses.c
+++ clang/test/Sema/parentheses.c
@@ -14,6 +14,18 @@
   if ((i = 4)) {}
 }
 
+_Bool return_assign(void) {
+  int i;
+  return i = 4; // expected-warning {{assignment as a condition}} \
+                // expected-note{{place parentheses around the assignment to silence this warning}} \
+                // expected-note{{use '==' to turn this assignment into an equality comparison}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:10-[[@LINE-3]]:10}:"("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:15-[[@LINE-4]]:15}:")"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-5]]:12-[[@LINE-5]]:13}:"=="
+
+  return (i = 4);
+}
+
 void bitwise_rel(unsigned i) {
   (void)(i & 0x2 == 0); // expected-warning {{& has lower precedence than ==}} \
                         // expected-note{{place parentheses around the '==' expression to silence this warning}} \
Index: clang/lib/Sema/SemaExprCXX.cpp
===================================================================
--- clang/lib/Sema/SemaExprCXX.cpp
+++ clang/lib/Sema/SemaExprCXX.cpp
@@ -3926,6 +3926,11 @@
     FromType = From->getType();
   }
 
+  // Warn on assignments inside implicit casts (= instead of ==)
+  if (SCS.Second == ICK_Boolean_Conversion || FromType == Context.BoolTy) {
+    DiagnoseAssignmentAsCondition(From->IgnoreImpCasts());
+  }
+
   // If we're converting to an atomic type, first convert to the corresponding
   // non-atomic type.
   QualType ToAtomicType;
Index: clang/lib/Sema/SemaExpr.cpp
===================================================================
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -16375,7 +16375,10 @@
 
 ExprResult Sema::CheckBooleanCondition(SourceLocation Loc, Expr *E,
                                        bool IsConstexpr) {
-  DiagnoseAssignmentAsCondition(E);
+  // C++ implicit casts are checked inside PerformImplicitConversion
+  if (!getLangOpts().CPlusPlus) {
+    DiagnoseAssignmentAsCondition(E);
+  }
   if (ParenExpr *parenE = dyn_cast<ParenExpr>(E))
     DiagnoseEqualityWithExtraParens(parenE);
 
Index: clang/lib/Sema/SemaChecking.cpp
===================================================================
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -9700,6 +9700,13 @@
           << FD << getLangOpts().CPlusPlus11;
     }
   }
+
+  // C++ implicit casts are checked inside PerformImplicitConversion
+  if (!getLangOpts().CPlusPlus) {
+    if (ImplicitCastExpr *CastExpr = dyn_cast<ImplicitCastExpr>(RetValExp)) {
+      DiagnoseAssignmentAsCondition(CastExpr->IgnoreImpCasts());
+    }
+  }
 }
 
 //===--- CHECK: Floating-Point comparisons (-Wfloat-equal) ---------------===//


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D63089.203866.patch
Type: text/x-patch
Size: 3568 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190610/5132ca7c/attachment.bin>