[PATCH] D108695: [analyzer][NFCI] Allow clients of NoStateChangeFuncVisitor to check entire function calls, rather than each ExplodedNode in it

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Nice work!



================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:361
+  while (!SCtx->inTopFrame()) {
+    auto p = FramesModifying.insert(SCtx);
+    if (!p.second)
----------------
Why don't we add the stack frame here to `FramesModifyingCalculated` as well? If we are in a nested stack frame and we step back to a parent then it would be redundant to do the calculation again for the parent.

Ahh, to be honest, I think the idea of having both `FramesModifying` and `FramesModifyingCalculated` separated is prone to errors.
Why don't we have a simple map<StackFramContext, bool> with the boolean true meaning that the frame is modifying? And if the frame is not in the map that means it had never been calculated before.


================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:370
+  while (N && !N->getLocationAs<CallExitEnd>())
+    N = N->getFirstSucc();
+  return N;
----------------
Szelethus wrote:
> NoQ wrote:
> > This is the right successor because we're in a heavily trimmed exploded graph, right?
> Should be, yes.
Would it make sense to assert that there are no more than one successor ?


================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:386
+
+  while (CurrN) {
+    // Found a new inlined call.
----------------
Using a `for` could simplify this loop and would eliminate the need to have a redundant loop variable bump both at line 420 and at 393.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108695/new/

https://reviews.llvm.org/D108695