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