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<ParenExpr>(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<ParenExpr>(E)) DiagnoseEqualityWithExtraParens(parenE);
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits