LegalizeAdulthood marked 10 inline comments as done.
LegalizeAdulthood added inline comments.
================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:386
- bool BoolValue = Bool->getValue();
+ const bool BoolValue = Bool->getValue();
----------------
JonasToth wrote:
> `const` on values is uncommon in clang-tidy code. Please keep that consistent.
I can change the code, but I don't understand the push back.
"That's the way it's done elsewhere" just doesn't seem like good reasoning.
I write const on values to signify that they are computed once and only once.
What is gained by removing that statement of once-and-only-once?
================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:533-540
+ switchStmt(has(
+ compoundStmt(
+ has(defaultStmt(hasDescendant(ifStmt(hasThen(returnsBool(Value)),
+ unless(hasElse(stmt())))
+ .bind(CompoundIfId)))
+ .bind(DefaultId)),
+ has(returnStmt(has(cxxBoolLiteral(equals(!Value))))))
----------------
aaron.ballman wrote:
> The check duplication here is unfortunate -- can you factor out the
> `hasDescendant()` bits into a variable that is reused, and perhaps use
> `anyOf(caseStmt(stuff()), defaultStmt(stuff()))` rather than separate
> functions?
I'm not a fan of duplication, either.
However, I have to know if it's a CaseStmt or DefaultStmt in the replacement
code and that's tied to the id, so I'm not sure how to collapse it further.
================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:719
+ bool Negated, const SwitchCase *SwitchCase) {
+ assert(SwitchCase != nullptr);
+
----------------
aaron.ballman wrote:
> Add a message to the assertion (same with the other ones).
I'm not sure what you're asking for. I based these asserts off the existing
assert in the file.
================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:743
+ // if the next statement after the if contains a return statement of
+ // the correct form. Ideally we'd be able to express this with the
+ // matchers, but that is currently impossible.
----------------
JonasToth wrote:
> double space
What exactly is the problem?
================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:745
+ // matchers, but that is currently impossible.
+ //
+ const auto *If = dyn_cast<IfStmt>(SwitchCase->getSubStmt());
----------------
JonasToth wrote:
> redundant empty comment line
Meh, it's not redundant. It's there to aid readability of a big block of text
by visually separating it from the associated code.
Why is this a problem?
================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:747
+ const auto *If = dyn_cast<IfStmt>(SwitchCase->getSubStmt());
+ assert(If != nullptr);
+ const CXXBoolLiteralExpr *Lit = stmtReturnsBool(If, Negated);
----------------
JonasToth wrote:
> I think this assertion does not hold true from the matcher.
> The matcher requires only `hasDescendent(ifStmt())`, but that does not ensure
> the first stmt is the `ifStmt`.
>
> e.g.
> ```
> case 10: {
> loggingCall();
> if(foo) ...
> ```
>
> Is this excluded?
> }
Look more carefully at the AST. CaseStmt has exactly one child. That child
can either be a compound statement block (which was already correctly handled
by the check) or it can be a single statement. This change enhances the check
to handle the single statement child of the CaseStmt and DefaultStmt.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56303/new/
https://reviews.llvm.org/D56303
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
