[clang-tidy] SimplifyBoolenExpr doesn't add parens if unary negotiation is of ExprWithCleanups type

[zinovy.nis]

Summary

bool foo(A &S) {
  if (S != (A)S)
    return false;
  return true;
}

is fixed into (w/o this patch)

...
return !S != (A)S; // negotiation affects first operand only
}

instead of (with this patch)

...
return S == (A)S; // note == instead of !=
}

[aaron.ballman]

LGTM, with a small nit.

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

78

const auto *, please.

[zinovy.nis]

• More accurate place to fix.

[malcolm.parsons]

clang-tools-extra/trunk/clang-tidy/readability/SimplifyBooleanExprCheck.cpp

198 ↗

(On Diff #148046)

E->IgnoreImplicit() can be used to ignore ExprWithCleanups

[zinovy.nis]

clang-tools-extra/trunk/clang-tidy/readability/SimplifyBooleanExprCheck.cpp

198 ↗

(On Diff #148046)

Thanks. But it seems to be too agressive:

return (i & 1) != 0;

becomes

return static_cast<bool>(i & 1);

[zinovy.nis]

clang-tools-extra/trunk/clang-tidy/readability/SimplifyBooleanExprCheck.cpp

198 ↗

(On Diff #148046)

if (!isa<ImplicitCastExpr>(E))
  E = E->IgnoreImplicit();

works properly but looks a bit verbose. What do you think?

[malcolm.parsons]

clang-tools-extra/trunk/clang-tidy/readability/SimplifyBooleanExprCheck.cpp

198 ↗

(On Diff #148046)

I think what you've committed is fine.

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

auto SimpleReturnsBool =
       returnStmt(has(cxxBoolLiteral(equals(Value)).bind(Id)))
           .bind("returns-bool");
   return anyOf(SimpleReturnsBool,
                compoundStmt(statementCountIs(1), has(SimpleReturnsBool)));
 }
 
 bool needsParensAfterUnaryNegation(const Expr *E) {
   E = E->IgnoreImpCasts();
   if (isa<BinaryOperator>(E) || isa<ConditionalOperator>(E))
     return true;
 
   if (const auto *Op = dyn_cast<CXXOperatorCallExpr>(E))
     return Op->getNumArgs() == 2 && Op->getOperator() != OO_Call &&
            Op->getOperator() != OO_Subscript;
 
   return false;
 }

 std::string compareExpressionToZero(const MatchFinder::MatchResult &Result,
                                     const Expr *E, bool Negated) {
   return compareExpressionToConstant(Result, E, Negated, "0");
 }
 
 std::string replacementExpression(const MatchFinder::MatchResult &Result,
                                   bool Negated, const Expr *E) {
   E = E->ignoreParenBaseCasts();
+  if (const auto *EC = dyn_cast<ExprWithCleanups>(E))
+    E = EC->getSubExpr();
+
   const bool NeedsStaticCast = needsStaticCast(E);
   if (Negated) {
     if (const auto *UnOp = dyn_cast<UnaryOperator>(E)) {
       if (UnOp->getOpcode() == UO_LNot) {
         if (needsNullPtrComparison(UnOp->getSubExpr()))
           return compareExpressionToNullPtr(Result, UnOp->getSubExpr(), true);
 
         if (needsZeroComparison(UnOp->getSubExpr()))

test/clang-tidy/readability-simplify-bool-expr.cpp

bool integer_member_implicit_cast(A *p) {
   if (p->m) {
     return true;
   } else {
     return false;
   }
 }
 // CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
 // CHECK-FIXES: return p->m != 0;{{$}}
+
+bool operator!=(const A&, const A&) { return false; }
+bool expr_with_cleanups(A &S) {
+  if (S != (A)S)
+    return false;
+
+  return true;
+}
+// CHECK-MESSAGES: :[[@LINE-4]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: S == (A)S;{{$}}