[NFCI][analyzer] Simplify NodeBuilder constructors (llvm#181875)

This commit simplifies the construction of `NodeBuilder` and its
subclasses in three ways:
- It removes an assertion that only appeared in one of the two
constructors of `NodeBuilder`. While the asserted "no sinks in the
`Frontier`" invariant apparently holds, this assertion was not the right
place for expressing it. (In the future I might re-add a similar
assertion in a more reasonable location.)
- It adds a new constructor for `NodeBuilder` that doesn't take a
"source node(s)" argument, because this argument was often irrelevant.
- It removes the "source node(s)" arguments from the subclasses of
`NodeBuilder` because it was always completely irrelevant in those
situations.

Before this commit, constructors of `NodeBuilder` took three arguments:
- the source node or nodes,
- the destination node set where the freshly built nodes are placed,
- the `NodeBuilderContext` that provides access to the exploded graph.

Among these, the latter two were saved in data members, but the only use
of the source node(s) was that the constructor inserted them into the
destination node set (= the data member `Frontier`).

However, adding the source node(s) to the `Frontier` is usually
irrelevant, because later set operations almost always remove them from
the `Frontier`. This is especially clear in the subclasses derived from
`NodeBuilder` whose constructor immediately remove the source node(s)
from the `Frontier`. (In other situations, the source node(s) are
usually removed from the `Frontier` by the calls to `generateNode()`.)

This commit introduces a new constructor for `NodeBuilder` that doesn't
have a "source node(s)" parameter, then uses this constructor to
implement the constructors of the subclasses without pointless insertion
and then removal of nodes from the destination set.

Note that if a node `N` was already present in the destination set, then
its insertion+removal would remove it from the set; but I verified that
the destination set passed to constructors of subclasses of
`NodeBuilder` is always an empty set (at this point).

The new constructor of `NodeBuiler` is public because later it will be
also useful for simplifying other code that uses `NodeBuilder` directly.

Author: NagyDonat

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h

@@ -245,30 +245,25 @@ class NodeBuilder {
   /// the builder dies.
   ExplodedNodeSet &Frontier;
 
-  bool hasNoSinksInFrontier() {
-    for (const auto  I : Frontier)
-      if (I->isSink())
-        return false;
-    return true;
-  }
-
   ExplodedNode *generateNodeImpl(const ProgramPoint &PP,
                                  ProgramStateRef State,
                                  ExplodedNode *Pred,
                                  bool MarkAsSink = false);
 
 public:
+  NodeBuilder(ExplodedNodeSet &DstSet, const NodeBuilderContext &Ctx)
+      : C(Ctx), Frontier(DstSet) {}
+
   NodeBuilder(ExplodedNode *SrcNode, ExplodedNodeSet &DstSet,
               const NodeBuilderContext &Ctx)
-      : C(Ctx), Frontier(DstSet) {
+      : NodeBuilder(DstSet, Ctx) {
     Frontier.Add(SrcNode);
   }
 
   NodeBuilder(const ExplodedNodeSet &SrcSet, ExplodedNodeSet &DstSet,
               const NodeBuilderContext &Ctx)
-      : C(Ctx), Frontier(DstSet) {
+      : NodeBuilder(DstSet, Ctx) {
     Frontier.insert(SrcSet);
-    assert(hasNoSinksInFrontier());
   }
 
   /// Generates a node in the ExplodedGraph.
@@ -333,21 +328,9 @@ class BranchNodeBuilder : public NodeBuilder {
   const CFGBlock *DstF;
 
 public:
-  BranchNodeBuilder(ExplodedNode *SrcNode, ExplodedNodeSet &DstSet,
-                    const NodeBuilderContext &C, const CFGBlock *DT,
-                    const CFGBlock *DF)
-      : NodeBuilder(SrcNode, DstSet, C), DstT(DT), DstF(DF) {
-    // The branch node builder does not generate autotransitions.
-    // If there are no successors it means that both branches are infeasible.
-    takeNodes(SrcNode);
-  }
-
-  BranchNodeBuilder(const ExplodedNodeSet &SrcSet, ExplodedNodeSet &DstSet,
-                    const NodeBuilderContext &C, const CFGBlock *DT,
-                    const CFGBlock *DF)
-      : NodeBuilder(SrcSet, DstSet, C), DstT(DT), DstF(DF) {
-    takeNodes(SrcSet);
-  }
+  BranchNodeBuilder(ExplodedNodeSet &DstSet, const NodeBuilderContext &C,
+                    const CFGBlock *DT, const CFGBlock *DF)
+      : NodeBuilder(DstSet, C), DstT(DT), DstF(DF) {}
 
   ExplodedNode *generateNode(ProgramStateRef State, bool branch,
                              ExplodedNode *Pred);
@@ -358,14 +341,9 @@ class IndirectGotoNodeBuilder : public NodeBuilder {
   const Expr *Target;
 
 public:
-  IndirectGotoNodeBuilder(ExplodedNode *SrcNode, ExplodedNodeSet &DstSet,
-                          NodeBuilderContext &Ctx, const Expr *Tgt,
-                          const CFGBlock *Dispatch)
-      : NodeBuilder(SrcNode, DstSet, Ctx), DispatchBlock(*Dispatch),
-        Target(Tgt) {
-    // The indirect goto node builder does not generate autotransitions.
-    takeNodes(SrcNode);
-  }
+  IndirectGotoNodeBuilder(ExplodedNodeSet &DstSet, NodeBuilderContext &Ctx,
+                          const Expr *Tgt, const CFGBlock *Dispatch)
+      : NodeBuilder(DstSet, Ctx), DispatchBlock(*Dispatch), Target(Tgt) {}
 
   using iterator = CFGBlock::const_succ_iterator;
 
@@ -386,12 +364,8 @@ class IndirectGotoNodeBuilder : public NodeBuilder {
 
 class SwitchNodeBuilder : public NodeBuilder {
 public:
-  SwitchNodeBuilder(ExplodedNode *SrcNode, ExplodedNodeSet &DstSet,
-                    const NodeBuilderContext &Ctx)
-      : NodeBuilder(SrcNode, DstSet, Ctx) {
-    // The switch node builder does not generate autotransitions.
-    takeNodes(SrcNode);
-  }
+  SwitchNodeBuilder(ExplodedNodeSet &DstSet, const NodeBuilderContext &Ctx)
+      : NodeBuilder(DstSet, Ctx) {}
 
   using iterator = CFGBlock::const_succ_reverse_iterator;
 

clang/lib/StaticAnalyzer/Core/CoreEngine.cpp

@@ -428,7 +428,7 @@ void CoreEngine::HandleBlockExit(const CFGBlock * B, ExplodedNode *Pred) {
         NodeBuilderContext Ctx(*this, B, Pred);
         ExplodedNodeSet Dst;
         IndirectGotoNodeBuilder Builder(
-            Pred, Dst, Ctx, cast<IndirectGotoStmt>(Term)->getTarget(),
+            Dst, Ctx, cast<IndirectGotoStmt>(Term)->getTarget(),
             *(B->succ_begin()));
 
         ExprEng.processIndirectGoto(Builder, Pred);

clang/lib/StaticAnalyzer/Core/ExprEngine.cpp

@@ -1650,7 +1650,7 @@ void ExprEngine::processCleanupTemporaryBranch(const CXXBindTemporaryExpr *BTE,
                                                ExplodedNodeSet &Dst,
                                                const CFGBlock *DstT,
                                                const CFGBlock *DstF) {
-  BranchNodeBuilder TempDtorBuilder(Pred, Dst, BldCtx, DstT, DstF);
+  BranchNodeBuilder TempDtorBuilder(Dst, BldCtx, DstT, DstF);
   ProgramStateRef State = Pred->getState();
   const LocationContext *LC = Pred->getLocationContext();
   if (getObjectUnderConstruction(State, BTE, LC)) {
@@ -2850,7 +2850,7 @@ void ExprEngine::processBranch(
 
   // Check for NULL conditions; e.g. "for(;;)"
   if (!Condition) {
-    BranchNodeBuilder NullCondBldr(Pred, Dst, BldCtx, DstT, DstF);
+    BranchNodeBuilder NullCondBldr(Dst, BldCtx, DstT, DstF);
     NullCondBldr.generateNode(Pred->getState(), true, Pred);
     return;
   }
@@ -2870,7 +2870,7 @@ void ExprEngine::processBranch(
   if (CheckersOutSet.empty())
     return;
 
-  BranchNodeBuilder Builder(CheckersOutSet, Dst, BldCtx, DstT, DstF);
+  BranchNodeBuilder Builder(Dst, BldCtx, DstT, DstF);
   for (ExplodedNode *PredN : CheckersOutSet) {
     if (PredN->isSink())
       continue;
@@ -2979,7 +2979,7 @@ void ExprEngine::processStaticInitializer(
   const auto *VD = cast<VarDecl>(DS->getSingleDecl());
   ProgramStateRef state = Pred->getState();
   bool initHasRun = state->contains<InitializedGlobalsSet>(VD);
-  BranchNodeBuilder Builder(Pred, Dst, BuilderCtx, DstT, DstF);
+  BranchNodeBuilder Builder(Dst, BuilderCtx, DstT, DstF);
 
   if (!initHasRun) {
     state = state->add<InitializedGlobalsSet>(VD);
@@ -3108,7 +3108,7 @@ void ExprEngine::processSwitch(NodeBuilderContext &BC, const SwitchStmt *Switch,
                                ExplodedNode *Pred, ExplodedNodeSet &Dst) {
   const Expr *Condition = Switch->getCond();
 
-  SwitchNodeBuilder Builder(Pred, Dst, BC);
+  SwitchNodeBuilder Builder(Dst, BC);
 
   ProgramStateRef State = Pred->getState();
   SVal CondV = State->getSVal(Condition, Pred->getLocationContext());