[PATCH] D56323: [clang-tidy] Handle member variables in readability-simplify-boolean-expr

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Thanks for the patch. Is this revision dependent on D56303 <https://reviews.llvm.org/D56303> (or other way around)?



================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:464
                                                   bool Value, StringRef Id) {
-  auto SimpleThen = binaryOperator(
-      hasOperatorName("="),
-      hasLHS(declRefExpr(hasDeclaration(decl().bind(IfAssignObjId)))),
-      hasLHS(expr().bind(IfAssignVariableId)),
-      hasRHS(cxxBoolLiteral(equals(Value)).bind(IfAssignLocId)));
-  auto Then = anyOf(SimpleThen, compoundStmt(statementCountIs(1),
-                                             hasAnySubstatement(SimpleThen)));
-  auto SimpleElse = binaryOperator(
-      hasOperatorName("="),
-      hasLHS(declRefExpr(hasDeclaration(equalsBoundNode(IfAssignObjId)))),
-      hasRHS(cxxBoolLiteral(equals(!Value))));
-  auto Else = anyOf(SimpleElse, compoundStmt(statementCountIs(1),
-                                             hasAnySubstatement(SimpleElse)));
+  const auto VarAssign =
+      declRefExpr(hasDeclaration(decl().bind(IfAssignVarId)));
----------------
The `const` for these locals in uncommon in clang-tidy, given its a value type. I am not strongly against them, but would prefer consistency.


================
Comment at: test/clang-tidy/readability-simplify-bool-expr-members.cpp:4
+struct X {
+  explicit operator bool();
+};
----------------
I didn't see a test that utilized this struct, did I overlook it?

Implicit conversion to `bool`-cases would be interesting as well. Maybe implicit conversion to integral in general.


================
Comment at: test/clang-tidy/readability-simplify-bool-expr-members.cpp:35
+  bool m_b3 = false;
+  bool m_b4 = false;
+  int *m_p = nullptr;
----------------
Would bitfields with a single bit be of interest as well?


================
Comment at: test/clang-tidy/readability-simplify-bool-expr-members.cpp:160
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: {{.*}} in ternary expression result
+  // CHECK-FIXES: {{^  m_b2 = i <= 20;$}}
+
----------------
For this case the logical inversion is simple an obviously `<=`, but what if the condition is more complex?

I would personally prefer `!(i < 20)` instead + complex logical expressions as additional test. They would be interesting for the `if` cases, too.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56323/new/

https://reviews.llvm.org/D56323