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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth 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)));
----------------
LegalizeAdulthood wrote:
> 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.
Ok, that would be great.


================
Comment at: test/clang-tidy/readability-simplify-bool-expr-members.cpp:4
+struct X {
+  explicit operator bool();
+};
----------------
LegalizeAdulthood wrote:
> 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()
Which other file? The other revision?


================
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;$}}
+
----------------
LegalizeAdulthood wrote:
> 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.
But the current version is the enhancement.
The complex condition, like `i < 20 && i > 0` will be the common case, so the inversion (with parens) should always be used (`!(i < 20 && i > 0)`) and specializations can be done later.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56323