[clang] [LifetimeSafety] Refactor FactGenerator to use RecursiveASTVisitor (PR #153661)

[Yitzhak Mandelbaum via cfe-commits]

================
@@ -403,29 +404,20 @@ class FactManager {
   llvm::BumpPtrAllocator FactAllocator;
 };
 
-class FactGenerator : public ConstStmtVisitor<FactGenerator> {
-  using Base = ConstStmtVisitor<FactGenerator>;
+class FactGeneratorVisitor : public ConstStmtVisitor<FactGeneratorVisitor> {
+  using Base = ConstStmtVisitor<FactGeneratorVisitor>;
 
 public:
-  FactGenerator(FactManager &FactMgr, AnalysisDeclContext &AC)
-      : FactMgr(FactMgr), AC(AC) {}
+  FactGeneratorVisitor(FactManager &FactMgr) : FactMgr(FactMgr) {}
 
-  void run() {
-    llvm::TimeTraceScope TimeProfile("FactGenerator");
-    // Iterate through the CFG blocks in reverse post-order to ensure that
-    // initializations and destructions are processed in the correct sequence.
-    for (const CFGBlock *Block : *AC.getAnalysis<PostOrderCFGView>()) {
-      CurrentBlockFacts.clear();
-      for (unsigned I = 0; I < Block->size(); ++I) {
-        const CFGElement &Element = Block->Elements[I];
-        if (std::optional<CFGStmt> CS = Element.getAs<CFGStmt>())
-          Visit(CS->getStmt());
-        else if (std::optional<CFGAutomaticObjDtor> DtorOpt =
-                     Element.getAs<CFGAutomaticObjDtor>())
-          handleDestructor(*DtorOpt);
-      }
-      FactMgr.addBlockFacts(Block, CurrentBlockFacts);
-    }
+  void startBlock(const CFGBlock *Block) {
+    CurrentBlock = Block;
+    CurrentBlockFacts.clear();
+  }
+
+  void endBlock() {
+    FactMgr.addBlockFacts(CurrentBlock, CurrentBlockFacts);
+    startBlock(nullptr);
----------------
ymand wrote:

Feels like misuse of the API. thoughts on adding an explicit clear() method and calling that or just inlining those 2 lines?
Also, why is clearing state on end necessary, given that you clear/set it on start?

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