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
