aaron.ballman added a comment. In D71846#1798115 <https://reviews.llvm.org/D71846#1798115>, @njames93 wrote:
> Sorry I didn't make it obvious with the test cases. The fix will never extend > the lifetime of variables either in the condition or declared in the else > block. It will only apply if the if is the last statement in its scope. > Though I should check back through the scope for any declarations that have > the same identifier as those in the if statement which would cause an error > if they were brought out of the if scope Oh, thank you for the clarification, I hadn't picked up on that. ================ Comment at: clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp:21 +static const char ReturnStr[] = "return"; +static const char ContinueStr[] = "continue"; ---------------- Hmm, I just realized we missed `co_return` for this check. That's a separate patch, though, so don't feel the need to change anything about this here. ================ Comment at: clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp:45 +const DeclRefExpr *findUsage(const Stmt *Node, int64_t DeclIdentifier) { + if (const auto *DeclRef = dyn_cast_or_null<DeclRefExpr>(Node)) { + if (DeclRef->getDecl()->getID() == DeclIdentifier) { ---------------- The use of `dyn_cast_or_null` is suspicious in these methods -- if `Node` is null, then we'll get into the `else` clause and promptly dereference a null pointer. My guess is that these should be `dyn_cast` calls instead, but if `Node` can truly be null, that case should be handled. ================ Comment at: clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp:50 + } else { + for (const auto &ChildNode : Node->children()) { + if (auto Result = findUsage(ChildNode, DeclIdentifier)) { ---------------- `const auto *`, no? (Same below.) ================ Comment at: clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp:51 + for (const auto &ChildNode : Node->children()) { + if (auto Result = findUsage(ChildNode, DeclIdentifier)) { + return Result; ---------------- `const auto *` for sure. (Same below.) ================ Comment at: clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp:77 +const DeclRefExpr *checkInitDeclUsageInElse(const IfStmt *If) { + auto InitDeclStmt = dyn_cast_or_null<DeclStmt>(If->getInit()); + if (!InitDeclStmt) ---------------- `const auto *` (this comment applies throughout the patch -- we don't want to deduce qualifiers or pointer/references, so they should be spelled out explicitly.) ================ Comment at: clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp:86 + llvm::SmallVector<int64_t, 4> DeclIdentifiers; + for (const auto &Decl : InitDeclStmt->decls()) { + assert(isa<VarDecl>(Decl) && "Init Decls must be a VarDecl"); ---------------- `const auto *`, no? ================ Comment at: clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp:123 + Diag << tooling::fixit::createRemoval(ElseLoc); + const auto LBrace = CS->getLBracLoc(); + const auto RBrace = CS->getRBracLoc(); ---------------- Please don't use `auto` here as the type is not spelled out in the initialization. Also, drop the top-level `const` qualifier. Same elsewhere. ================ Comment at: clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp:161 + if (!IsLastInScope && containsDeclInScope(Else)) { + // warn, but don't attempt an autofix + diag(ElseLoc, WarningMessage) << ControlFlowInterruptor; ---------------- warn -> Warn and should have a full stop at the end of the comment. The same applies to the other instances of this comment. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71846/new/ https://reviews.llvm.org/D71846 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits