[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:511
+                                                      StringRef Id) {
+  // binding to the returnStmt matched is pointless because we can't guarantee
+  // anything about the ordering of the return statement and the case statement.
----------------
binding -> Binding


================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:529
+                                                         StringRef Id) {
+  // binding to the returnStmt matched is pointless because we can't guarantee
+  // anything about the ordering of the return statement and the case statement.
----------------
binding -> Binding

Also, there is no "case statement" involved here. ;-)


================
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))))))
----------------
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?


================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:718
+    const MatchFinder::MatchResult &Result, const CompoundStmt *Compound,
+    bool Negated, const SwitchCase *SwitchCase) {
+  assert(SwitchCase != nullptr);
----------------
Don't use an identifier with the same name as a type, please.


================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:719
+    bool Negated, const SwitchCase *SwitchCase) {
+  assert(SwitchCase != nullptr);
+
----------------
Add a message to the assertion (same with the other ones).


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56303/new/

https://reviews.llvm.org/D56303