[PATCH] D45401: Fix 31480 - warn more aggressively when assignments get used as a boolean values

[Will Song via Phabricator via cfe-commits]

incertia created this revision.
incertia added reviewers: rjmccall, rsmith.
incertia added a project: clang.
Herald added a subscriber: cfe-commits.

To my understanding, the contents of a condition will always be implicitly 
casted to an rvalue before evaluation. Thus, moving the check to 
PerformImplicitConversion should be correct. We can also check the more general 
case here of general boolean expressions. This should catch a few more cases vs 
gcc. The only examples that I can come up with that will bypass this check 
would be

  bool & f(bool &b) { return b = true; }
  f(b = false); // b = false is an lvalue, the reference passed into the 
function is an lvalue, and the reference returned out is an lvalue, which 
causes no warnings.

Clearly passing in the result of an assignment to a function is not very good, 
so we should probably have a separate check for that. On the other hand, I have 
no idea if we should even warn on returning the (boolean) result of assigning 
to a boolean reference or how such a check would be implemented, outside of 
brute force within ActOnReturnStmt, which would collide with the warnings in 
PerformImplicitConversion.


Repository:
  rC Clang

https://reviews.llvm.org/D45401

Files:
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaExprCXX.cpp


Index: lib/Sema/SemaExprCXX.cpp
===
--- lib/Sema/SemaExprCXX.cpp
+++ lib/Sema/SemaExprCXX.cpp
@@ -3860,6 +3860,10 @@
 
   case ICK_Lvalue_To_Rvalue: {
 assert(From->getObjectKind() != OK_ObjCProperty);
+if (SCS.Second == ICK_Boolean_Conversion ||
+From->getType() == Context.BoolTy) {
+  DiagnoseAssignmentAsCondition(From->IgnoreImpCasts());
+}
 ExprResult FromRes = DefaultLvalueConversion(From);
 assert(!FromRes.isInvalid() && "Can't perform deduced conversion?!");
 From = FromRes.get();
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -15409,7 +15409,10 @@
 
 ExprResult Sema::CheckBooleanCondition(SourceLocation Loc, Expr *E,
bool IsConstexpr) {
-  DiagnoseAssignmentAsCondition(E);
+  // an assignment always produces an lvalue, and we now check this inside
+  // PerformImplicitConversion, as conditions will always be implicitly
+  // converted to an rvalue
+  // DiagnoseAssignmentAsCondition(E);
   if (ParenExpr *parenE = dyn_cast(E))
 DiagnoseEqualityWithExtraParens(parenE);
 


Index: lib/Sema/SemaExprCXX.cpp
===
--- lib/Sema/SemaExprCXX.cpp
+++ lib/Sema/SemaExprCXX.cpp
@@ -3860,6 +3860,10 @@
 
   case ICK_Lvalue_To_Rvalue: {
 assert(From->getObjectKind() != OK_ObjCProperty);
+if (SCS.Second == ICK_Boolean_Conversion ||
+From->getType() == Context.BoolTy) {
+  DiagnoseAssignmentAsCondition(From->IgnoreImpCasts());
+}
 ExprResult FromRes = DefaultLvalueConversion(From);
 assert(!FromRes.isInvalid() && "Can't perform deduced conversion?!");
 From = FromRes.get();
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -15409,7 +15409,10 @@
 
 ExprResult Sema::CheckBooleanCondition(SourceLocation Loc, Expr *E,
bool IsConstexpr) {
-  DiagnoseAssignmentAsCondition(E);
+  // an assignment always produces an lvalue, and we now check this inside
+  // PerformImplicitConversion, as conditions will always be implicitly
+  // converted to an rvalue
+  // DiagnoseAssignmentAsCondition(E);
   if (ParenExpr *parenE = dyn_cast(E))
 DiagnoseEqualityWithExtraParens(parenE);
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45401: Fix 31480 - warn more aggressively when assignments get used as a boolean values

[Will Song via Phabricator via cfe-commits]

incertia updated this revision to Diff 141477.
incertia added a comment.

Pulled the check outside of the case statement.


https://reviews.llvm.org/D45401

Files:
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaExprCXX.cpp


Index: lib/Sema/SemaExprCXX.cpp
===
--- lib/Sema/SemaExprCXX.cpp
+++ lib/Sema/SemaExprCXX.cpp
@@ -3839,6 +3839,11 @@
 FromType = From->getType();
   }
 
+  // implicit boolean conversions should have their assignments checked
+  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: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -15409,7 +15409,10 @@
 
 ExprResult Sema::CheckBooleanCondition(SourceLocation Loc, Expr *E,
bool IsConstexpr) {
-  DiagnoseAssignmentAsCondition(E);
+  // an assignment always produces an lvalue, and we now check this inside
+  // PerformImplicitConversion, as conditions will always be implicitly
+  // converted to an rvalue
+  // DiagnoseAssignmentAsCondition(E);
   if (ParenExpr *parenE = dyn_cast(E))
 DiagnoseEqualityWithExtraParens(parenE);
 


Index: lib/Sema/SemaExprCXX.cpp
===
--- lib/Sema/SemaExprCXX.cpp
+++ lib/Sema/SemaExprCXX.cpp
@@ -3839,6 +3839,11 @@
 FromType = From->getType();
   }
 
+  // implicit boolean conversions should have their assignments checked
+  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: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -15409,7 +15409,10 @@
 
 ExprResult Sema::CheckBooleanCondition(SourceLocation Loc, Expr *E,
bool IsConstexpr) {
-  DiagnoseAssignmentAsCondition(E);
+  // an assignment always produces an lvalue, and we now check this inside
+  // PerformImplicitConversion, as conditions will always be implicitly
+  // converted to an rvalue
+  // DiagnoseAssignmentAsCondition(E);
   if (ParenExpr *parenE = dyn_cast(E))
 DiagnoseEqualityWithExtraParens(parenE);
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits