[cfe-dev] fix for Clang PR 8419

[Ted Kremenek]

On Nov 20, 2010, at 1:24 AM, Zhanyong Wan (λx.x x) wrote:

>> -void GRExprEngine::EvalLoad(ExplodedNodeSet& Dst, const Expr *Ex,
>> +void GRExprEngine::EvalLoad(ExplodedNodeSet& Dst, const Expr *Ex,
>>                              ExplodedNode* Pred,
>>                              const GRState* state, SVal location,
>>                              const void *tag, QualType LoadTy) {
>> +  assert(!isa<NonLoc>(location) && "location cannot be a NonLoc.");
>> 
>> Is it possible to change 'location' to a Loc, instead of having this
>> assertion?
> 
> That was my first attempt.  Unfortunately, it turned out that
> 'location' could also be an undefined or unknown SVal (I actually see
> such values when debugging).  Not sure if that's a bug -- in any case
> I'd rather not attacking it right now.

Right.  The assertion makes sense here.

> 
>> +  VisitLValue(Ex, Pred, Tmp);  // This modifies Tmp.
>> 
>> Do you think this comment is necessary?  This happens all over the place
>> within the engine.
> 
> It was helpful to me.  The problem with
> 
>  VisitLValue(Ex, Pred, Tmp);
> 
> is that there's no way to tell from the call site alone which
> arguments are the input and which are the output.  This made the code
> much harder to read to me, as I had to maintain more context in my
> brain ("remember that Tmp is passed by non-const reference and will be
> modified").
> 
> I'll remove the comment.  In a future patch, I'd like to change the
> signature of the functions to pass output parameters by pointer as
> opposed to by reference.  In other words, the call will look like
> 
>  VisitLValue(Ex, Pred, &Tmp);
> 
> which gives a clear hint that Tmp could be modified.

Understood.  As a compromise, what I would prefer is something more like:

  VisitLValue(Ex, Pred, /* generated nodes = */ &Tmp);

This follows similar convention elsewhere in the codebase for documenting parameter behavior.  Following your other suggestions, probably more suggestive in the variables names, or better API design.

Otherwise the changes look great!