[cfe-commits] r110191 - in /cfe/trunk: include/clang/Checker/PathSensitive/Checker.h include/clang/Checker/PathSensitive/GRExprEngine.h lib/Checker/GRExprEngine.cpp lib/Checker/MallocChecker.cpp

[Jordy Rose]

On Wed, 4 Aug 2010 09:07:33 -0700, Ted Kremenek <kremenek at apple.com>
wrote:
> Hi Jordy,
> 
> I think this is a good cleanup/improvement, but I'm not a fan of the
added
> 'respondsToCallback' to the Checker::EvalAssume:
> 
>> const GRState *MallocChecker::EvalAssume(const GRState *state, SVal
>> Cond,
>> -                                         bool Assumption) {
>> +                                         bool Assumption,
>> +                                         bool * /* respondsToCallback
>> */) {
> 
> This extra parameter is really a turd in the interface; the Checker
should
> never need to reason about it.  With the other callbacks where we do
this
> optimization, we pass a 'CheckerContext' object (which contains the
flag),
> but that isn't really needed here since we aren't creating
ExplodedNodes. 
> If we ever need to pass some kind of context object here, it would be
great
> to sink that flag into it (at some point).

I agree, but I didn't have a better solution, and EvalAssume is a pretty
rare case. (It's also the most similar to a hypothetical EvalRegionChange.)
It was mostly proof-of-concept for the tweaks to COCache, that they'd be
straightforward to use for a non-statement-based callback.

Everything I think of ends up needing cooperation from the subclasser: the
extra argument, an llvm::Optional return value, etc. I suppose we could
have a special sentinel value that's not a valid pointer (1? ~0?) that's
only returned by the base class. If we did that, we could wrap it in a
GR_EvalAssume, which just uses a bool pointer as before.

This is not a general solution, though. Alternately, we could just stick
the flag in Checker rather than CheckerContext.