https://github.com/NagyDonat created https://github.com/llvm/llvm-project/pull/204187
Replace four `NodeBuilder`s with direct use of the `makeNode` method family. This is part of my commit series that simplifies the engine by gradually eliminating the class `NodeBuilder`. ------- "Why is this NFC" justifications can be found in the descriptions of the individual commits. @necto This change touches the method `ProcessLifetimeEnd` which was recently published by you. I don't know much you already know about this refactoring effort (which I started several months ago), but if you are not yet familiar with it, I think this PR is a good and clear example of what I'm doing in this long commit series. For the motivation behind this project, you can see [this comment](https://github.com/llvm/llvm-project/pull/181431#issuecomment-3900458198) and the commit message of #182377 -- and I'm happy to answer any remaining questions. From 651de90d902cac7227a9cecaa3c845c72cca535c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Tue, 16 Jun 2026 13:22:16 +0200 Subject: [PATCH 1/5] Eliminate NodeBuilder from ProcessLifetimeEnd The NodeBuilder in this recently published method was used in a simple and straightforward way, so I was able to replace it with a `makeNode` call. Note that the new code relies on passing a single `ExplodedNode *` to an `ExplodedNodeSet` parameter of `runCheckersForLifetimeEnd` -- this works because there is a converting constructor which is frequently used to streamline situations like this. --- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp index 32da5e097c76e..0d2fa88ec0ec1 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -1148,10 +1148,8 @@ void ExprEngine::ProcessLifetimeEnd(const Stmt *S, const VarDecl *D, PrettyStackTraceLoc CrashInfo(getContext().getSourceManager(), S->getBeginLoc(), "Error evaluating end of a lifetime"); - ExplodedNodeSet Src; - NodeBuilder Bldr(Pred, Src, *currBldrCtx); LifetimeEnd PP(S, D, Pred->getStackFrame()); - Bldr.generateNode(PP, Pred->getState(), Pred); + ExplodedNode *Src = Engine.makeNode(PP, Pred->getState(), Pred); ExplodedNodeSet Dst; getCheckerManager().runCheckersForLifetimeEnd(Dst, Src, D, *this); From 13005af677d06ca221aed88e4e2711d1a13451cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Tue, 16 Jun 2026 13:27:57 +0200 Subject: [PATCH 2/5] Eliminate NodeBuilder from VisitCXXBindTemporaryExpr The constructor of this `NodeBuilder` was inserting all elements of `PreVisit` to `Dst` (the `Frontier` of the `NodeBuilder`), but then the `for` loop removed each element when it unconditionally called `generateNode` from each `Node`. (Note that `Dst` was empty previously, this method is only called from `ExprEngine::Visit`.) The new code elides this back-and-forth logic by only inserting the freshly created nodes into `Dst`. --- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp index 0d2fa88ec0ec1..a1c600cfc1980 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -1637,7 +1637,6 @@ void ExprEngine::VisitCXXBindTemporaryExpr(const CXXBindTemporaryExpr *BTE, Dst = PreVisit; return; } - NodeBuilder Builder(PreVisit, Dst, *currBldrCtx); for (ExplodedNode *Node : PreVisit) { ProgramStateRef State = Node->getState(); const StackFrame *SF = Node->getStackFrame(); @@ -1648,7 +1647,7 @@ void ExprEngine::VisitCXXBindTemporaryExpr(const CXXBindTemporaryExpr *BTE, // temporary destructor nodes. State = addObjectUnderConstruction(State, BTE, SF, UnknownVal()); } - Builder.generateNode(BTE, Node, State); + Dst.insert(Engine.makePostStmtNode(BTE, State, Node)); } } From e29599ee7bd999e47049ef9edcb2e0c366d79d39 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Tue, 16 Jun 2026 13:50:12 +0200 Subject: [PATCH 3/5] Eliminate NodeBuilder in VisitMemberExpr This was a slightly more complex case: the constructor of the `NodeBuilder` inserts `CheckedSet` into `EvalSet` (which was empty previously), then the loop iterating over `CheckedSet` has a complex body, but still each iteration reaches either a `takeNodes` or a `generateNode` call that removes the current node `I` from `EvalSet`. Therefore I was able to "cancel out" the insertion side effect of the constructor and these node removal operations. The new code just creates the new nodes with the higher-level `makeNodeWithBinding` (that simplifies the code) and directly inserts them into `EvalSet`. I could probably already remove `ExplodedNodeSet &Tmp` in this commit, but right now I'm just adding a FIXME because understanding the potential effect of this change will be simpler when there are no `NodeBuilder`s in `evalLoad`. --- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp index a1c600cfc1980..7a8ccfc02889d 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -3349,7 +3349,6 @@ void ExprEngine::VisitMemberExpr(const MemberExpr *M, ExplodedNode *Pred, for (const auto I : CheckedSet) VisitCommonDeclRefExpr(M, Member, I, EvalSet); } else { - NodeBuilder Bldr(CheckedSet, EvalSet, *currBldrCtx); ExplodedNodeSet Tmp; for (const auto I : CheckedSet) { @@ -3363,9 +3362,8 @@ void ExprEngine::VisitMemberExpr(const MemberExpr *M, ExplodedNode *Pred, state = createTemporaryRegionIfNeeded(state, SF, BaseExpr); SVal MDVal = svalBuilder.getFunctionPointer(MD); - state = state->BindExpr(M, SF, MDVal); - Bldr.generateNode(M, I, state); + EvalSet.insert(Engine.makeNodeWithBinding(I, M, MDVal, state)); continue; } @@ -3409,12 +3407,13 @@ void ExprEngine::VisitMemberExpr(const MemberExpr *M, ExplodedNode *Pred, L = UnknownVal(); } - Bldr.generateNode(M, I, state->BindExpr(M, SF, L), nullptr, - ProgramPoint::PostLValueKind); + EvalSet.insert(Engine.makeNodeWithBinding( + I, M, L, state, ProgramPoint::PostLValueKind)); } else { - Bldr.takeNodes(I); + // FIXME: When evalLoad no longer uses NodeBuilders, eliminate Tmp and + // pass EvalSet as the first argument of evalLoad. evalLoad(Tmp, M, M, I, state, L); - Bldr.addNodes(Tmp); + EvalSet.insert(Tmp); } } } From b18b0144e34b88f1ac392ca893dbb86d4cde1f88 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Tue, 16 Jun 2026 14:04:55 +0200 Subject: [PATCH 4/5] Eliminate NodeBuilder from VisitAtomicExpr The constructor of this `NodeBuilder` was inserting all elements of `AfterPreSet` to the previously empty local `AfterInvalidateSet` (the `Frontier` of the `NodeBuilder`), but then the `for` loop removed each element when it unconditionally called `generateNode` from each `I`. The new code elides this back-and-forth logic by only inserting the freshly created nodes into `AfterInvalidateSet`. (This node creation matches the standard pattern of `makeNodeWithBinding`, so I was able to simplify the code.) --- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp index 7a8ccfc02889d..7c9490e6b8646 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -3430,7 +3430,6 @@ void ExprEngine::VisitAtomicExpr(const AtomicExpr *AE, ExplodedNode *Pred, // FIXME: Ideally we should model the behavior of the atomics precisely here. ExplodedNodeSet AfterInvalidateSet; - NodeBuilder Bldr(AfterPreSet, AfterInvalidateSet, *currBldrCtx); for (const auto I : AfterPreSet) { ProgramStateRef State = I->getState(); @@ -3448,10 +3447,8 @@ void ExprEngine::VisitAtomicExpr(const AtomicExpr *AE, ExplodedNode *Pred, /*CausedByPointerEscape*/ true, /*Symbols=*/nullptr); - SVal ResultVal = UnknownVal(); - State = State->BindExpr(AE, SF, ResultVal); - Bldr.generateNode(AE, I, State, nullptr, - ProgramPoint::PostStmtKind); + AfterInvalidateSet.insert( + Engine.makeNodeWithBinding(I, AE, UnknownVal(), State)); } getCheckerManager().runCheckersForPostStmt(Dst, AfterInvalidateSet, AE, *this); From a0d26d9ff05ca5aeb354b50208a07e2c7ef736f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Tue, 16 Jun 2026 14:13:22 +0200 Subject: [PATCH 5/5] [side] Use range-based loop over children of AtomicExpr We do have `AtomicExpr::children` that returns a range -- unfortunately this range contains the subexpressions as `Stmt*` objects so I needed to use a `cast` to get a proper `Expr*`. --- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp index 7c9490e6b8646..0ecef1c77f4cb 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -3436,9 +3436,8 @@ void ExprEngine::VisitAtomicExpr(const AtomicExpr *AE, ExplodedNode *Pred, const StackFrame *SF = I->getStackFrame(); SmallVector<SVal, 8> ValuesToInvalidate; - for (unsigned SI = 0, Count = AE->getNumSubExprs(); SI != Count; SI++) { - const Expr *SubExpr = AE->getSubExprs()[SI]; - SVal SubExprVal = State->getSVal(SubExpr, SF); + for (const Stmt *SubExpr : AE->children()) { + SVal SubExprVal = State->getSVal(cast<Expr>(SubExpr), SF); ValuesToInvalidate.push_back(SubExprVal); } _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
