[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:
> k-wisniewski wrote:
> > NoQ wrote:
> > > 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.
> > How about the situation when this callback is triggered by something other than a call, like variable assignment? I've tried using that location context and had lots of segfaults as in such cases it appeared to be null. Do you have some info that it should be non-null in such a case?
> Ok, makes sense. Have you looked into providing a checker context there? How much more difficult would that be? If that is too difficult, adding LocationContext is good as well.
> 
> If Call is optional and LocationContext is not, should the Call parameter be last.
If we add a CheckerContext, then we may end up calling callbacks that split states within callbacks that split states, and that'd be quite a mess to support.

Right now a checker may trigger `checkRegionChanges()` within its callback by manipulating the Store, which already leads to callbacks within callbacks, but that's bearable as long as you can't add transitions within the inner callbacks.


https://reviews.llvm.org/D26588