[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 (auto *EC = dyn_cast<ExprWithCleanups>(E))
+    E = EC->getSubExpr();
   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;

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; }
+extern bool GetValue(A s);
+bool expr_with_cleanups(A &s) {
+  A S;
+  if (GetValue(s) && S != (A)s)
+    return false;
+
+  return true;
+}
+// CHECK-MESSAGES: :[[@LINE-4]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: !(GetValue(s) && S != (A)s);{{$}}