[cfe-commits] [PATCH] First step towards adding a parent map to the ASTContext.

[Manuel Klimek]

On Wed, Jan 16, 2013 at 6:19 PM, Jordan Rose <jordan_rose at apple.com> wrote:

>
>   Hm. I'm worried about the expense of looking up a single parent for an
> arbitrary Stmt* here. In the current libAST version of ParentMap, It's just
> a simple DenseMap lookup. Now, it's a wrap/unwrap in a DynTypedNode,
> followed by a lookup, followed by a "get first" or "get last" on the
> ParentVector.
>
>   I'm not saying this isn't necessary in the general case, but it is a hot
> part of the analyzer. I'd feel better if we were sure the wrap/unwrap could
> be optimized away.
>
>   Also, what's the motivation behind returning the ParentVector by value?
> How about an ArrayRef? That way there's no permanence implied, but it still
> takes constant time and doesn't copy anything.
>
>   As far as the interface goes, well, you can see all the convenience
> methods we have on the current ParentMap. `isConsumedExpr` is the most
> important, but the recursive `getParentIgnore*` methods are also useful.
>

I'd be all for adding more convenience methods in a follow-up CL, and also
adding more support for the more specific use cases of the static analyzer.
For example, for the static analyzer I'm not sure at all that caching makes
sense or is needed - we might just want to have a method that creates the
parent map (containing all the nifty convenience methods) from a limited
scope (that is, usually a declaration node).

I'm not sure yet whether we need to answer that question before getting the
more "generic" interface in place (of course I agree I should investigate
your performance concerns first). I think there has been requests for the
generic interface on this list a few times (from people not using tooling,
but hacking on AST visitors). For the AST matchers, we're currently facing
quite some performance problem without the caching, so I'm hopeful we get a
go/no-go decision on this path earlier rather than later, and I'm committed
to following through with the needs of the static analyzer :)


>   Like I said before, ParentMap is mostly used for location info and to
> see whether or not an expression is top-level, so we can //probably// get
> away with using an arbitrary instantiation parent most of the time in both
> the analyzer and the migrator(s).
>

Which might lead back to it being better if the static analyzer controls
its own parent map, especially if it is in the hot path (?)

Thanks!
/Manuel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130116/590080c7/attachment.html>