On Jan 16, 2013, at 12:00 , Manuel Klimek <[email protected]> wrote:
> On Wed, Jan 16, 2013 at 6:19 PM, Jordan Rose <[email protected]> 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 :) It seems sensible for that purpose, modulo Doug's comment on IRC that eager deserialization of every imported PCH / module seems like a bad idea. > 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 (?) Yeah, at least the hot path part could be moved from libAST to libAnalysis or libStaticAnalyzerCore. The non-hot-path (diagnostic emission) could probably be retrofitted to use the new ParentMap. Jordan
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
