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

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood marked 4 inline comments as done.
LegalizeAdulthood added inline comments.


================
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)));
----------------
JonasToth wrote:
> 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.
I can undo the const.


================
Comment at: test/clang-tidy/readability-simplify-bool-expr-members.cpp:4
+struct X {
+  explicit operator bool();
+};
----------------
JonasToth wrote:
> 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.
Leftover from copy/paste of readability-simplify-bool-expr.cpp, I'll remove it.

Implicit conversion cases are covered by the other file.  Here, I'm just testing the cases that interact with member variables because the matcher needs to use memberExpr() and not just declRefExpr()


================
Comment at: test/clang-tidy/readability-simplify-bool-expr-members.cpp:35
+  bool m_b3 = false;
+  bool m_b4 = false;
+  int *m_p = nullptr;
----------------
JonasToth wrote:
> Would bitfields with a single bit be of interest as well?
That could be an enhancement, but it's not addressed by this patch.


================
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;$}}
+
----------------
JonasToth wrote:
> 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.
If you have complex conditions, you can run the check repeatedly until you reach a fixed point.

Using !(x) instead of the reverse operator could be added as an enhancement, but it is not covered by this patch.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56323