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,
----------------
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.
================
Comment at: include/clang/StaticAnalyzer/Core/Checker.h:333
+ ProgramStateRef state,
+ const LocationContext *LCtx) {
+ return ((const CHECKER *) checker)->wantsRegionChangeUpdate(state, LCtx);
----------------
k-wisniewski wrote:
> NoQ wrote:
> > 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).
> Well I've added it to be consistent with checkRegionChanges, but AFAIK this
> callback is no longer necessary and there was a discussion about getting rid
> of it altogether. Maybe it's a good moment ot purge it?
I don't recall where that discussion has ended. If the callback is not used,
I'd be fine with removing it. (That would need to be a separate patch.)
https://reviews.llvm.org/D26588
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
