[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

[Aaron Ballman via Phabricator via cfe-commits]

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