<html><head><meta http-equiv="Content-Type" content="text/html charset=iso-8859-1"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Jan 16, 2013, at 12:00 , Manuel Klimek <<a href="mailto:klimek@google.com">klimek@google.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div dir="ltr"><div class="gmail_default" style="">On Wed, Jan 16, 2013 at 6:19 PM, Jordan Rose <span dir="ltr"><<a href="mailto:jordan_rose@apple.com" target="_blank">jordan_rose@apple.com</a>></span> wrote:<br></div><div class="gmail_extra">
<div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
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.<br>
<br>
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.<br>
<br>
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.<br>
<br>
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.<br></blockquote>
<div><br></div><div style="">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).</div>
<div style=""><br></div><div style="">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 :)</div>
<div></div></div></div></div></blockquote><div><br></div><div>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.</div><div><br></div><br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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).<br>
</blockquote><div><br></div><div style="">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 (?)</div></div></div></div></blockquote><br></div><div>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.</div><div><br></div><div>Jordan</div></body></html>