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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
