[PATCH] D56303: [clang-tidy] Handle case/default statements when simplifying boolean expressions
Aaron Ballman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Sun Jan 6 07:43:27 PST 2019
aaron.ballman added inline comments.
================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:386
- bool BoolValue = Bool->getValue();
+ const bool BoolValue = Bool->getValue();
----------------
LegalizeAdulthood wrote:
> 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?
> "That's the way it's done elsewhere" just doesn't seem like good reasoning.
Keeping the code consistent with the vast majority of the remainder of the project is valid reasoning. I am echoing the request to drop the top-level const. We don't use this pattern for non-pointer/reference types and there's not a whole lot gained by using it inconsistently.
================
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))))))
----------------
LegalizeAdulthood wrote:
> 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.
You can bind to the `castStmt()` and `defaultStmt()` matchers to see what you get back, no? e.g., `anyOf(caseStmt(stuff()).bind("which"), defaultStmt(stuff()).bind("which"))` and in the check, you can use `isa<>` on the node named `"which"` to see whether you matched the case or the default label.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56303/new/
https://reviews.llvm.org/D56303
More information about the cfe-commits
mailing list