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