[cfe-commits] Checker and respondsToCallback

[Jordy Rose]

Checkers aren't currently stateless, but they probably ought to be much
more so, especially for external inlining.

Anyway, since Assume triggers ProcessAssume, we already have a problem --
many many checkers use Assume. We could take the optimization out of these
"subordinate" checks, but that sort of defeats the purpose.

Maybe CheckerContext needs to be a hierarchy. We could make the base
context very small (perhaps just the flag) and add to it for the various
callbacks. It's a little silly because it's sort of shadowing the
callbacks, but it could solve two problems: having a local context for
each
invocation to store the respondsToCallback flag, and having a common
method
signature for all the callbacks, to allow us to package the
visit-and-cache
logic in its own method(s). (A possible solution to the problem discussed
on another thread.)


On Thu, 5 Aug 2010 21:18:21 -0700, Ted Kremenek <kremenek at apple.com>
wrote:
> I think it's fair to require that Checker callbacks can't call each
other.
> If they need to call shared logic, they can use an internal, non-virtual
> function.
> 
> I think the SaveAndRestore is also not necessary in ResetCallbackFlag. 
If
> GRExprEngine cares about the callback flag, it will reset it before
calling
> the callback.  There is no need to restore it.
> 
> One disadvantage that I realized about putting this flag in Checker is
> that it makes checkers less thread-safe.  I'm not certain what direction
we
> are heading it, but if Checkers were mostly stateless they could
possibly
> be used by multiple GRExprEngines in multiple threads.  From this
> perspective, putting the flag in the CheckerContext object is better.
> 
> On Aug 5, 2010, at 12:14 PM, Jordy Rose wrote:
> 
>> Hm. Thanks to the fact that one callback may end up indirectly invoking
>> another, this is not so simple. On the one hand, it does let us track
the
>> response for any callback. On the other, ResetCallbackFlag is a kludge.
>> 
>> Holding off on committing for now.
>> 
>> 
>> On Wed, 4 Aug 2010 17:10:17 -0700, Ted Kremenek <kremenek at apple.com>
>> wrote:
>>> That's not a bad idea at all.  In fact I really like it.  The value
can
>>> still get lazily set, but just stored with the Checker object.
>>> 
>>> On Aug 4, 2010, at 10:51 AM, Jordy Rose wrote:
>>> 
>>>> This is not a general solution, though. Alternately, we could just
>> stick
>>>> the flag in Checker rather than CheckerContext.
>> <Checker-respondsToCallback.patch>