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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
