NagyDonat wrote: > Let me forward couple seemingly relevant comments from my agent: > > * CoreEngine::makeNode is const, so the existing const CoreEngine > &getCoreEngine() const already suffices for addTransitionImpl (that's how > NodeBuilder::generateNode reached it). Is > the non-const overload deliberate — signalling that node creation > mutates the graph — or can it be dropped so CheckerContext.h doesn't expose a > mutable CoreEngine to every checker?
The non-const overload is indeed superfluous, I'll remove it. I have misremembered the signature of `makeNode` (which was added by me a few months ago) – I thought that it is a non-const method. > * ExplodedNode still has friend class NodeBuilder; (ExplodedGraph.h:69), > keeping the name alive as an incomplete type. Delete it here? Look for > similar stale spellings. I tried to remove references to `NodeBuilder` with a "Jump to references" command in my clangd-powered IDE. > > * The currBldrCtx comment block and now-unused getBuilderContext() — fold > into this PR, or leave to the announced NodeBuilderContext follow-up? > > * On your own EndFunction question: the asymmetry looks worth fixing, but > as a separate non-NFC change with a test — please keep it out of this PR. https://github.com/llvm/llvm-project/pull/217319 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
