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 <[email protected]> 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 <[email protected]> >> 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> _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
