[clang] [LifetimeSafety] Refactor buildOriginFlowChain to use PImpl pattern (PR #201071)

[via cfe-commits]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang-analysis

Author: Utkarsh Saxena (usx95)

<details>
<summary>Changes</summary>

Move `buildOriginFlowChain` into `AnalysisImpl`, removing `FactManager` parameter

---
Full diff: https://github.com/llvm/llvm-project/pull/201071.diff


3 Files Affected:

- (modified) clang/include/clang/Analysis/Analyses/LifetimeSafety/LoanPropagation.h (+2-2) 
- (modified) clang/lib/Analysis/LifetimeSafety/LoanPropagation.cpp (+46-40) 
- (modified) clang/unittests/Analysis/LifetimeSafetyTest.cpp (-1) 


``````````diff
diff --git a/clang/include/clang/Analysis/Analyses/LifetimeSafety/LoanPropagation.h b/clang/include/clang/Analysis/Analyses/LifetimeSafety/LoanPropagation.h
index 168223c42e2b2..e46912bedeb0f 100644
--- a/clang/include/clang/Analysis/Analyses/LifetimeSafety/LoanPropagation.h
+++ b/clang/include/clang/Analysis/Analyses/LifetimeSafety/LoanPropagation.h
@@ -45,8 +45,8 @@ class LoanPropagationAnalysis {
   /// sequence of origins through which the loan flowed, ending at the origin
   /// where the loan was originally issued.
   llvm::SmallVector<OriginID>
-  buildOriginFlowChain(const FactManager &FactMgr, ProgramPoint StartPoint,
-                       const OriginID StartOID, const LoanID TargetLoan) const;
+  buildOriginFlowChain(ProgramPoint StartPoint, const OriginID StartOID,
+                       const LoanID TargetLoan) const;
 
 private:
   class Impl;
diff --git a/clang/lib/Analysis/LifetimeSafety/LoanPropagation.cpp b/clang/lib/Analysis/LifetimeSafety/LoanPropagation.cpp
index cc24829281214..8c570cd29673b 100644
--- a/clang/lib/Analysis/LifetimeSafety/LoanPropagation.cpp
+++ b/clang/lib/Analysis/LifetimeSafety/LoanPropagation.cpp
@@ -198,6 +198,47 @@ class AnalysisImpl
     return getLoans(getState(P), OID);
   }
 
+  llvm::SmallVector<OriginID>
+  buildOriginFlowChain(ProgramPoint StartPoint, const OriginID StartOID,
+                       const LoanID TargetLoan) const {
+    assert(getLoans(StartOID, StartPoint).contains(TargetLoan) &&
+           "TargetLoan must be present in the StartOID at the StartPoint");
+
+    OriginID CurrOID = StartOID;
+    llvm::SmallVector<OriginID> OriginFlowChain;
+    llvm::ArrayRef<const Fact *> Facts = FactMgr.getBlockContaining(StartPoint);
+    const auto *StartIt = llvm::find(Facts, StartPoint);
+    assert(StartIt != Facts.end());
+
+    for (const Fact *F :
+         llvm::reverse(llvm::make_range(Facts.begin(), StartIt))) {
+      if (const auto *IF = F->getAs<IssueFact>())
+        if (IF->getLoanID() == TargetLoan) {
+          assert(IF->getOriginID() == CurrOID);
+          return OriginFlowChain;
+        }
+
+      const auto *OFF = F->getAs<OriginFlowFact>();
+      if (!OFF)
+        continue;
+      if (OFF->getDestOriginID() != CurrOID)
+        continue;
+
+      const OriginID SrcOriginID = OFF->getSrcOriginID();
+      if (!getLoans(SrcOriginID, OFF).contains(TargetLoan))
+        continue;
+      OriginFlowChain.push_back(SrcOriginID);
+      CurrOID = SrcOriginID;
+    }
+
+    // FIXME: Ideally, this return is unreachable and should be an assert
+    // because we expect to always finish at an IssueFact. But since current
+    // traversal is limited to a single CFG block, multi-block OriginFlowChain
+    // construction might miss the IssueFact. We should add llvm_unreachable
+    // here once multi-block support is implemented.
+    return {};
+  }
+
 private:
   /// Returns true if the origin is persistent (referenced in multiple blocks).
   bool isPersistent(OriginID OID) const {
@@ -248,45 +289,10 @@ LoanSet LoanPropagationAnalysis::getLoans(OriginID OID, ProgramPoint P) const {
   return PImpl->getLoans(OID, P);
 }
 
-llvm::SmallVector<OriginID> LoanPropagationAnalysis::buildOriginFlowChain(
-    const FactManager &FactMgr, ProgramPoint StartPoint,
-    const OriginID StartOID, const LoanID TargetLoan) const {
-  assert(getLoans(StartOID, StartPoint).contains(TargetLoan) &&
-         "TargetLoan must be present in the StartOID at the StartPoint");
-
-  OriginID CurrOID = StartOID;
-  llvm::SmallVector<OriginID> OriginFlowChain;
-  llvm::ArrayRef<const Fact *> Facts = FactMgr.getBlockContaining(StartPoint);
-  const auto *StartIt = llvm::find(Facts, StartPoint);
-  assert(StartIt != Facts.end());
-
-  for (const Fact *F :
-       llvm::reverse(llvm::make_range(Facts.begin(), StartIt))) {
-    if (const auto *IF = F->getAs<IssueFact>())
-      if (IF->getLoanID() == TargetLoan) {
-        assert(IF->getOriginID() == CurrOID);
-        return OriginFlowChain;
-      }
-
-    const auto *OFF = F->getAs<OriginFlowFact>();
-    if (!OFF)
-      continue;
-    if (OFF->getDestOriginID() != CurrOID)
-      continue;
-
-    const OriginID SrcOriginID = OFF->getSrcOriginID();
-    if (!getLoans(SrcOriginID, OFF).contains(TargetLoan))
-      continue;
-    OriginFlowChain.push_back(SrcOriginID);
-    CurrOID = SrcOriginID;
-  }
-
-  // FIXME: Ideally, this return is unreachable and should be an assert because
-  // we expect to always finish at an IssueFact. But since current traversal is
-  // limited to a single CFG block, multi-block OriginFlowChain construction
-  // might miss the IssueFact.
-  // We should add llvm_unreachable here once multi-block support is
-  // implemented.
-  return {};
+llvm::SmallVector<OriginID>
+LoanPropagationAnalysis::buildOriginFlowChain(ProgramPoint StartPoint,
+                                              const OriginID StartOID,
+                                              const LoanID TargetLoan) const {
+  return PImpl->buildOriginFlowChain(StartPoint, StartOID, TargetLoan);
 }
 } // namespace clang::lifetimes::internal
diff --git a/clang/unittests/Analysis/LifetimeSafetyTest.cpp b/clang/unittests/Analysis/LifetimeSafetyTest.cpp
index 7105f3de2891e..febecb472455a 100644
--- a/clang/unittests/Analysis/LifetimeSafetyTest.cpp
+++ b/clang/unittests/Analysis/LifetimeSafetyTest.cpp
@@ -215,7 +215,6 @@ class LifetimeTestHelper {
     for (LoanID LID : EndLoanIDs) {
       const llvm::SmallVector<OriginID> OriginFlowChain =
           Runner.getAnalysis().getLoanPropagation().buildOriginFlowChain(
-              Runner.getAnalysis().getFactManager(),
               getProgramPoint(Annotation), *StartOriginID, LID);
       if (!OriginFlowChain.empty())
         return OriginFlowChain;

``````````

</details>


https://github.com/llvm/llvm-project/pull/201071