[PATCH] D26588: Add LocationContext to members of check::RegionChanges

[Artem Dergachev via cfe-commits]

NoQ added inline comments.


================
Comment at: include/clang/StaticAnalyzer/Core/Checker.h:325
+                      const CallEvent *Call,
+                      const LocationContext *LCtx) {
+    return ((const CHECKER *) checker)->checkRegionChanges(state, invalidated,
----------------
zaks.anna wrote:
> LocationContext can be obtained by calling CallEvent::getLocationContext(). I do not think that adding another parameter here buys us much. Am I missing something?
CallEvent* is optional here - it's there only for invalidations through calls.


================
Comment at: include/clang/StaticAnalyzer/Core/Checker.h:333
+                                       ProgramStateRef state,
+                                       const LocationContext *LCtx) {
+    return ((const CHECKER *) checker)->wantsRegionChangeUpdate(state, LCtx);
----------------
zaks.anna wrote:
> What is the scenario in which you need the context here? The idea is that the checker would be interested in region changes if it is already tracking information in the state. For that check, the information in the state is sufficient. What is your scenario?
> 
> Also, For any checker API change, we need to update the CheckerDocumentation.cpp.
Environment, which is a significant portion of the state, is essentially unaccessible without a location context.

Call stack is also an important bit of information to any checker that does something special to support/exploit the inlining IPA - there are many checkers that scan the call stack, but so far none of them needed to subscribe to checkRegionChanges; their possible use case may look like "we want an update only if a certain condition was met within the current stack frame".

For the recursion checker, i think, the point is "if current frame is spoiled, we no longer want updates" (which should probably be fixed in the checker patch: even though the callback is broken, it'd take one less action to fix it if we decide to).


https://reviews.llvm.org/D26588