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