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