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

[Richard via Phabricator via cfe-commits]

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