[analyzer][NFC] Cleanup BranchNodeBuilder (#117898)

Previously `BranchNodeBuilder` had a machinery to mark the two possible
branches (true, false) as infeasible, but this was completely useless in
practice, because the `BranchNodeBuilder` objects where short-lived
local variables so the `markInfeasible()` calls did not affect any later
calls.

The only theoretical exception was that in `ExprEngine::processBranch`
the methods of `BranchNodeBuilder` were called within a `for` loop that
iterates over the nodes created by the `check::BranchCondition`
callbacks.

However, currently only two checkers listen to `check::BranchCondition`
and neither of them produces multiple out nodes. This is fortunate,
because if the `for` loop had multiple iterations, then the lingering
effects of `markInfeasible()` would have caused wildly incorrect
behavior.

_For example, let's assume that a hypothetical `check::BranchCondition`
callback transitions to two different states, and the condition
expression happens to be true in the first and false in the second. In
this situation the first iteration of the loop would mark the false
branch as 'infeasible' and then in the second iteration the analyzer
would skip creating the transition to the false branch (from the state
where that is the 'right' path forward)._

After removing `markInfeasible()`, it became clear that the
`isFeasible()` calls in `ExprEngine::processBranch` are redundant
because they only guarded a `generateNode` call -- which immediately
calls `isFeasible()` and does nothing on an infeasible branch.

At this point in the refactoring the only code writing the feasibility
data members is their initialization in the constructors of
`BranchNodeBuilder` and the only code reading them is the check at the
beginning of `BranchNodeBuilder::generateNode`, so it was
straightforward to remove them completely and simplify the logic of
`generateNode()` to let it directly check the nullness of the
`CFGBlock*` pointer that it wants to use.

Finally, after these changes it became clear that in
`ExprEngine::processBranch` the `else` branch
(corresponding to the case when `assumeCondition` fails) is equivalent
to the "normal" case, so I eliminated it as well.

I also update the capitalization of a few variables that are already
affected by this change.

Author: NagyDonat

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

@@ -429,47 +429,27 @@ class BranchNodeBuilder: public NodeBuilder {
   const CFGBlock *DstT;
   const CFGBlock *DstF;
 
-  bool InFeasibleTrue;
-  bool InFeasibleFalse;
-
   void anchor() override;
 
 public:
   BranchNodeBuilder(ExplodedNode *SrcNode, ExplodedNodeSet &DstSet,
-                    const NodeBuilderContext &C,
-                    const CFGBlock *dstT, const CFGBlock *dstF)
-      : NodeBuilder(SrcNode, DstSet, C), DstT(dstT), DstF(dstF),
-        InFeasibleTrue(!DstT), InFeasibleFalse(!DstF) {
+                    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 *dstT, const CFGBlock *dstF)
-      : NodeBuilder(SrcSet, DstSet, C), DstT(dstT), DstF(dstF),
-        InFeasibleTrue(!DstT), InFeasibleFalse(!DstF) {
+                    const NodeBuilderContext &C, const CFGBlock *DT,
+                    const CFGBlock *DF)
+      : NodeBuilder(SrcSet, DstSet, C), DstT(DT), DstF(DF) {
     takeNodes(SrcSet);
   }
 
   ExplodedNode *generateNode(ProgramStateRef State, bool branch,
                              ExplodedNode *Pred);
-
-  const CFGBlock *getTargetBlock(bool branch) const {
-    return branch ? DstT : DstF;
-  }
-
-  void markInfeasible(bool branch) {
-    if (branch)
-      InFeasibleTrue = true;
-    else
-      InFeasibleFalse = true;
-  }
-
-  bool isFeasible(bool branch) {
-    return branch ? !InFeasibleTrue : !InFeasibleFalse;
-  }
 };
 
 class IndirectGotoNodeBuilder {

clang/lib/StaticAnalyzer/Core/CoreEngine.cpp

@@ -649,14 +649,15 @@ StmtNodeBuilder::~StmtNodeBuilder() {
 void BranchNodeBuilder::anchor() {}
 
 ExplodedNode *BranchNodeBuilder::generateNode(ProgramStateRef State,
-                                              bool branch,
+                                              bool Branch,
                                               ExplodedNode *NodePred) {
-  // If the branch has been marked infeasible we should not generate a node.
-  if (!isFeasible(branch))
+  const CFGBlock *Dst = Branch ? DstT : DstF;
+
+  if (!Dst)
     return nullptr;
 
-  ProgramPoint Loc = BlockEdge(C.getBlock(), branch ? DstT : DstF,
-                               NodePred->getLocationContext());
+  ProgramPoint Loc =
+      BlockEdge(C.getBlock(), Dst, NodePred->getLocationContext());
   ExplodedNode *Succ = generateNodeImpl(Loc, State, NodePred);
   return Succ;
 }

clang/lib/StaticAnalyzer/Core/ExprEngine.cpp

@@ -1647,10 +1647,8 @@ void ExprEngine::processCleanupTemporaryBranch(const CXXBindTemporaryExpr *BTE,
   ProgramStateRef State = Pred->getState();
   const LocationContext *LC = Pred->getLocationContext();
   if (getObjectUnderConstruction(State, BTE, LC)) {
-    TempDtorBuilder.markInfeasible(false);
     TempDtorBuilder.generateNode(State, true, Pred);
   } else {
-    TempDtorBuilder.markInfeasible(true);
     TempDtorBuilder.generateNode(State, false, Pred);
   }
 }
@@ -2770,7 +2768,6 @@ void ExprEngine::processBranch(const Stmt *Condition,
   // Check for NULL conditions; e.g. "for(;;)"
   if (!Condition) {
     BranchNodeBuilder NullCondBldr(Pred, Dst, BldCtx, DstT, DstF);
-    NullCondBldr.markInfeasible(false);
     NullCondBldr.generateNode(Pred->getState(), true, Pred);
     return;
   }
@@ -2790,40 +2787,25 @@ void ExprEngine::processBranch(const Stmt *Condition,
   if (CheckersOutSet.empty())
     return;
 
-  BranchNodeBuilder builder(CheckersOutSet, Dst, BldCtx, DstT, DstF);
+  BranchNodeBuilder Builder(CheckersOutSet, Dst, BldCtx, DstT, DstF);
   for (ExplodedNode *PredN : CheckersOutSet) {
     if (PredN->isSink())
       continue;
 
     ProgramStateRef PrevState = PredN->getState();
 
-    ProgramStateRef StTrue, StFalse;
+    ProgramStateRef StTrue = PrevState, StFalse = PrevState;
     if (const auto KnownCondValueAssumption = assumeCondition(Condition, PredN))
       std::tie(StTrue, StFalse) = *KnownCondValueAssumption;
-    else {
-      assert(!isa<ObjCForCollectionStmt>(Condition));
-      builder.generateNode(PrevState, true, PredN);
-      builder.generateNode(PrevState, false, PredN);
-      continue;
-    }
+
     if (StTrue && StFalse)
       assert(!isa<ObjCForCollectionStmt>(Condition));
 
-    // Process the true branch.
-    if (builder.isFeasible(true)) {
-      if (StTrue)
-        builder.generateNode(StTrue, true, PredN);
-      else
-        builder.markInfeasible(true);
-    }
+    if (StTrue)
+      Builder.generateNode(StTrue, true, PredN);
 
-    // Process the false branch.
-    if (builder.isFeasible(false)) {
-      if (StFalse)
-        builder.generateNode(StFalse, false, PredN);
-      else
-        builder.markInfeasible(false);
-    }
+    if (StFalse)
+      Builder.generateNode(StFalse, false, PredN);
   }
   currBldrCtx = nullptr;
 }
@@ -2845,14 +2827,13 @@ void ExprEngine::processStaticInitializer(const DeclStmt *DS,
   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(Pred, Dst, BuilderCtx, DstT, DstF);
 
   if (!initHasRun) {
     state = state->add<InitializedGlobalsSet>(VD);
   }
 
-  builder.generateNode(state, initHasRun, Pred);
-  builder.markInfeasible(!initHasRun);
+  Builder.generateNode(state, initHasRun, Pred);
 
   currBldrCtx = nullptr;
 }