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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
