[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:
> NoQ wrote:
> > 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.
> This argument by itself does not seem to be preventing us from providing CheckerContext. For example, we could disallow splitting states (ex: by setting some flag within the CheckerContext) but still provide most of the convenience APIs. 
> 
> The advantage of providing CheckerContext is that it is a class that provides access to different analyzer APIs that the checker writer might want to access. It is also familiar and somewhat expected as it is available in most other cases. It might be difficult to pipe enough information to construct it... I suspect that is the reason we do not provide it in this case.
> 
> I am OK with the patch as is but it would be good to explore if we could extend this API by providing the full CheckerContext.
Hmm, with this kind of plan we should probably move all transition functionality into a sub-class of `CheckerContext`(?) So that it was obvious from the signature that some of those are restricted, and some are full-featured.

This definitely sounds like a big task, useful though, maybe a separate patch over all callbacks might be a good idea.


https://reviews.llvm.org/D26588