[clang] [NFCI][analyzer] Simplify NodeBuilder constructors (PR #181875)
Donát Nagy via cfe-commits
cfe-commits at lists.llvm.org
Tue Feb 17 10:06:35 PST 2026
https://github.com/NagyDonat created https://github.com/llvm/llvm-project/pull/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.
>From 29455b95eb3119ecd5cf25a641697ead18aad159 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <donat.nagy at ericsson.com>
Date: Tue, 17 Feb 2026 13:25:03 +0100
Subject: [PATCH 1/2] [NFCI][analyzer] Remove an inconsistent assertion
Among the two constructors of `NodeBuilder`, the one that takes a
`SrcSet` asserted that the `Frontier` doesn't contain any sink nodes,
while the one that takes a single `SrcNode` didn't contain an analogous
assertion.
So far this assertion happened to be true (or at least I don't know
about any failures), but I don't see why is it important to assert this
here, so I'm removing the assertion to restore consistency between the
constructors and simplify the code.
---
.../clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h | 8 --------
1 file changed, 8 deletions(-)
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h
index a7fe34b425d5d..113c43187ade7 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h
@@ -245,13 +245,6 @@ 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,
@@ -268,7 +261,6 @@ class NodeBuilder {
const NodeBuilderContext &Ctx)
: C(Ctx), Frontier(DstSet) {
Frontier.insert(SrcSet);
- assert(hasNoSinksInFrontier());
}
/// Generates a node in the ExplodedGraph.
>From 11d029c2b6db70c1a2ea5764ef419f926c6bd92c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <donat.nagy at ericsson.com>
Date: Tue, 17 Feb 2026 15:57:04 +0100
Subject: [PATCH 2/2] [NFC][analyzer] Allow constructing NodeBuilders without
sources
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 almost never
relevant, 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 insertion+removal would remove the node from the destination
set if it was already present in the set before these operations; 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.
---
.../Core/PathSensitive/CoreEngine.h | 44 ++++++-------------
clang/lib/StaticAnalyzer/Core/CoreEngine.cpp | 2 +-
clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 10 ++---
3 files changed, 19 insertions(+), 37 deletions(-)
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h
index 113c43187ade7..d4669fd2d1720 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h
@@ -251,15 +251,18 @@ class NodeBuilder {
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);
}
@@ -325,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);
@@ -350,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;
@@ -378,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;
diff --git a/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp b/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
index 1e5a04b8efbe4..9f0fcc4c3e3a5 100644
--- a/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
+++ b/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);
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
index e0bc083f6b978..8c9265ae55311 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ b/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());
More information about the cfe-commits
mailing list