[cfe-dev] fix for Clang PR 8419

Zhanyong Wan (λx.x x) wan at google.com
Sat Nov 20 01:24:24 PST 2010


Please find my reply to your comments about the code comments I'm
adding below.  Thanks.

On Fri, Nov 19, 2010 at 1:54 PM, Ted Kremenek <kremenek at apple.com> wrote:
>
> +// FIXME: The name clang::Environment is too generic.  Find a more
> +// descriptive name (e.g. GREnvironment?).
>
> Instead of a FIXME, why not just do this change?

This patch has dragged on for too long, and I really want to limit the
scope of it.  If you think GREnvironment is a good name, I'll make the
change in a separate patch.

> +  /// Like LookupExpr, but doesn't require Ex to be in the map (in
> +  ///  which case the return value will either be UnknownVal() or
> +  ///  created by the ValueManager).
>    SVal GetSVal(const Stmt* Ex, ValueManager& ValMgr) const;
>
> This is a high-level, public API; this level of detail in the comment seems
> a little weird.  The fact that values are lazily generated is an
> implementation detail.  I'd rather the comment just say that this fetches
> the current binding of the expression in the Environment.  The fact that the
> binding is implicitly or explicitly represented in the Environment is an
> implementation detail.
> Also, LookupExpr is a private detail of the class.  Comparing GetSVal to
> LookupExpr is a little weird when it comes to documenting a method used by
> clients.

I'll remove the reference to LookupExpr.

Since the ValueManager object appears in the public API (as an
argument of the function), we cannot really say it's an implementation
detail (the user of this function needs to know what ValueManager
object to pass in and what the function will do to it -- even if he
doesn't care about how the function is implemented.)  Therefore I
think we need to explain what it's for.

> -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.

> +  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.

-- 
Zhanyong




More information about the cfe-dev mailing list