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

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus marked an inline comment as done.
Szelethus added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:361
+  while (!SCtx->inTopFrame()) {
+    auto p = FramesModifying.insert(SCtx);
+    if (!p.second)
----------------
martong wrote:
> 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.
Yes, its stupid, but its just the one thing I didn't want to bother myself with. Though I agree it'd be better to drop these sets for something a lot more elegant. Btw mind that we insert into the calculated set each time we enter a new stack frame.

I'll leave a TODO, if you don't mind.


================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:369
+static const ExplodedNode *getCallExitEnd(const ExplodedNode *N) {
+  while (N && !N->getLocationAs<CallExitEnd>())
+    N = N->getFirstSucc();
----------------
NoQ wrote:
> Wait, no, I don't think this test is sufficient. You may run into a `CallExitEnd` of a nested stack frame before you reach the `CallExitEnd` of the stack frame in question:
> ```
> -> CallEnter foo()       --->
>   -> CallEnter bar()        |
>   <- CallExitEnd bar()      <--- ???
> <- CallExitEnd foo()
> ```
Good point. Added a bunch of asserts and changed to code to both retrieve, and ensure that the correct node is retrieved.


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

https://reviews.llvm.org/D108695