[clang] [analyzer][NFC] Eliminate a dyn_cast (PR #100719)

Kristóf Umann via cfe-commits cfe-commits at lists.llvm.org
Mon Aug 5 03:08:22 PDT 2024


https://github.com/Szelethus updated https://github.com/llvm/llvm-project/pull/100719

>From d176b03b211144dadaa1efb4b7da959110d7725c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Krist=C3=B3f=20Umann?= <dkszelethus at gmail.com>
Date: Fri, 26 Jul 2024 11:01:41 +0200
Subject: [PATCH 1/2] [analyzer][NFC] Eliminate a dyn_cast

---
 clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp          | 5 ++---
 clang/lib/StaticAnalyzer/Checkers/NoOwnershipChangeVisitor.h | 2 +-
 clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp          | 5 ++---
 3 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index fe202c79ed620..39e5c7c014a2a 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -796,14 +796,13 @@ class NoMemOwnershipChangeVisitor final : public NoOwnershipChangeVisitor {
   /// done syntactically, because we are trying to argue about alternative
   /// paths of execution, and as a consequence we don't have path-sensitive
   /// information.
-  bool doesFnIntendToHandleOwnership(const Decl *Callee,
+  bool doesFnIntendToHandleOwnership(const FunctionDecl *Callee,
                                      ASTContext &ACtx) final {
     using namespace clang::ast_matchers;
-    const FunctionDecl *FD = dyn_cast<FunctionDecl>(Callee);
 
     auto Matches = match(findAll(stmt(anyOf(cxxDeleteExpr().bind("delete"),
                                             callExpr().bind("call")))),
-                         *FD->getBody(), ACtx);
+                         Callee->getBody(), ACtx);
     for (BoundNodes Match : Matches) {
       if (Match.getNodeAs<CXXDeleteExpr>("delete"))
         return true;
diff --git a/clang/lib/StaticAnalyzer/Checkers/NoOwnershipChangeVisitor.h b/clang/lib/StaticAnalyzer/Checkers/NoOwnershipChangeVisitor.h
index 027f1a156a7c0..7be74860d863b 100644
--- a/clang/lib/StaticAnalyzer/Checkers/NoOwnershipChangeVisitor.h
+++ b/clang/lib/StaticAnalyzer/Checkers/NoOwnershipChangeVisitor.h
@@ -26,7 +26,7 @@ class NoOwnershipChangeVisitor : public NoStateChangeFuncVisitor {
   /// is done syntactically, because we are trying to argue about alternative
   /// paths of execution, and as a consequence we don't have path-sensitive
   /// information.
-  virtual bool doesFnIntendToHandleOwnership(const Decl *Callee,
+  virtual bool doesFnIntendToHandleOwnership(const FunctionDecl *Callee,
                                              ASTContext &ACtx) = 0;
 
   virtual bool hasResourceStateChanged(ProgramStateRef CallEnterState,
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 9aee7f952ad2d..41187ee9b5cdf 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -746,13 +746,12 @@ class NoStreamStateChangeVisitor final : public NoOwnershipChangeVisitor {
     return StreamChk->FCloseDesc.matchesAsWritten(Call);
   }
 
-  bool doesFnIntendToHandleOwnership(const Decl *Callee,
+  bool doesFnIntendToHandleOwnership(const FunctionDecl *Callee,
                                      ASTContext &ACtx) final {
     using namespace clang::ast_matchers;
-    const FunctionDecl *FD = dyn_cast<FunctionDecl>(Callee);
 
     auto Matches =
-        match(findAll(callExpr().bind("call")), *FD->getBody(), ACtx);
+        match(findAll(callExpr().bind("call")), Callee->getBody(), ACtx);
     for (BoundNodes Match : Matches) {
       if (const auto *Call = Match.getNodeAs<CallExpr>("call"))
         if (isClosingCallAsWritten(*Call))

>From f96c21c018029bd314fefdbca7ea868490cb6992 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Krist=C3=B3f=20Umann?= <dkszelethus at gmail.com>
Date: Mon, 5 Aug 2024 12:08:11 +0200
Subject: [PATCH 2/2] Make users responsible for parsing Callee instead

---
 .../lib/StaticAnalyzer/Checkers/MallocChecker.cpp  | 14 ++++++++++++--
 .../Checkers/NoOwnershipChangeVisitor.cpp          | 10 ----------
 .../Checkers/NoOwnershipChangeVisitor.h            |  2 +-
 .../lib/StaticAnalyzer/Checkers/StreamChecker.cpp  | 14 ++++++++++++--
 4 files changed, 25 insertions(+), 15 deletions(-)

diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index 39e5c7c014a2a..3a13b45d9c440 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -796,13 +796,23 @@ class NoMemOwnershipChangeVisitor final : public NoOwnershipChangeVisitor {
   /// done syntactically, because we are trying to argue about alternative
   /// paths of execution, and as a consequence we don't have path-sensitive
   /// information.
-  bool doesFnIntendToHandleOwnership(const FunctionDecl *Callee,
+  bool doesFnIntendToHandleOwnership(const Decl *Callee,
                                      ASTContext &ACtx) final {
+    const FunctionDecl *FD = dyn_cast<FunctionDecl>(Callee);
+
+    // Given that the stack frame was entered, the body should always be
+    // theoretically obtainable. In case of body farms, the synthesized body
+    // is not attached to declaration, thus triggering the '!FD->hasBody()'
+    // branch. That said, would a synthesized body ever intend to handle
+    // ownership? As of today they don't. And if they did, how would we
+    // put notes inside it, given that it doesn't match any source locations?
+    if (!FD || !FD->hasBody())
+      return false;
     using namespace clang::ast_matchers;
 
     auto Matches = match(findAll(stmt(anyOf(cxxDeleteExpr().bind("delete"),
                                             callExpr().bind("call")))),
-                         Callee->getBody(), ACtx);
+                         *FD->getBody(), ACtx);
     for (BoundNodes Match : Matches) {
       if (Match.getNodeAs<CXXDeleteExpr>("delete"))
         return true;
diff --git a/clang/lib/StaticAnalyzer/Checkers/NoOwnershipChangeVisitor.cpp b/clang/lib/StaticAnalyzer/Checkers/NoOwnershipChangeVisitor.cpp
index 22b5ebfd6fab0..91f4ca371aa98 100644
--- a/clang/lib/StaticAnalyzer/Checkers/NoOwnershipChangeVisitor.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/NoOwnershipChangeVisitor.cpp
@@ -72,16 +72,6 @@ bool NoOwnershipChangeVisitor::wasModifiedInFunction(
     const ExplodedNode *CallEnterN, const ExplodedNode *CallExitEndN) {
   const Decl *Callee =
       CallExitEndN->getFirstPred()->getLocationContext()->getDecl();
-  const FunctionDecl *FD = dyn_cast<FunctionDecl>(Callee);
-
-  // Given that the stack frame was entered, the body should always be
-  // theoretically obtainable. In case of body farms, the synthesized body
-  // is not attached to declaration, thus triggering the '!FD->hasBody()'
-  // branch. That said, would a synthesized body ever intend to handle
-  // ownership? As of today they don't. And if they did, how would we
-  // put notes inside it, given that it doesn't match any source locations?
-  if (!FD || !FD->hasBody())
-    return false;
   if (!doesFnIntendToHandleOwnership(
           Callee,
           CallExitEndN->getState()->getAnalysisManager().getASTContext()))
diff --git a/clang/lib/StaticAnalyzer/Checkers/NoOwnershipChangeVisitor.h b/clang/lib/StaticAnalyzer/Checkers/NoOwnershipChangeVisitor.h
index 7be74860d863b..027f1a156a7c0 100644
--- a/clang/lib/StaticAnalyzer/Checkers/NoOwnershipChangeVisitor.h
+++ b/clang/lib/StaticAnalyzer/Checkers/NoOwnershipChangeVisitor.h
@@ -26,7 +26,7 @@ class NoOwnershipChangeVisitor : public NoStateChangeFuncVisitor {
   /// is done syntactically, because we are trying to argue about alternative
   /// paths of execution, and as a consequence we don't have path-sensitive
   /// information.
-  virtual bool doesFnIntendToHandleOwnership(const FunctionDecl *Callee,
+  virtual bool doesFnIntendToHandleOwnership(const Decl *Callee,
                                              ASTContext &ACtx) = 0;
 
   virtual bool hasResourceStateChanged(ProgramStateRef CallEnterState,
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 41187ee9b5cdf..32ec4cbc236f1 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -746,12 +746,22 @@ class NoStreamStateChangeVisitor final : public NoOwnershipChangeVisitor {
     return StreamChk->FCloseDesc.matchesAsWritten(Call);
   }
 
-  bool doesFnIntendToHandleOwnership(const FunctionDecl *Callee,
+  bool doesFnIntendToHandleOwnership(const Decl *Callee,
                                      ASTContext &ACtx) final {
+    const FunctionDecl *FD = dyn_cast<FunctionDecl>(Callee);
+
+    // Given that the stack frame was entered, the body should always be
+    // theoretically obtainable. In case of body farms, the synthesized body
+    // is not attached to declaration, thus triggering the '!FD->hasBody()'
+    // branch. That said, would a synthesized body ever intend to handle
+    // ownership? As of today they don't. And if they did, how would we
+    // put notes inside it, given that it doesn't match any source locations?
+    if (!FD || !FD->hasBody())
+      return false;
     using namespace clang::ast_matchers;
 
     auto Matches =
-        match(findAll(callExpr().bind("call")), Callee->getBody(), ACtx);
+        match(findAll(callExpr().bind("call")), *FD->getBody(), ACtx);
     for (BoundNodes Match : Matches) {
       if (const auto *Call = Match.getNodeAs<CallExpr>("call"))
         if (isClosingCallAsWritten(*Call))



More information about the cfe-commits mailing list