[PATCH] D56303: [clang-tidy] Handle case/default statements when simplifying boolean expressions

[Aaron Ballman via Phabricator via cfe-commits]

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