<div dir="ltr">Did you reply in phabricator or just on this email?<div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Mar 11, 2013 at 3:57 PM, Manuel Klimek <span dir="ltr"><<a href="mailto:klimek@google.com" target="_blank" class="cremed">klimek@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="im">On Mon, Mar 11, 2013 at 7:19 AM, Daniel Jasper <span dir="ltr"><<a href="mailto:djasper@google.com" target="_blank" class="cremed">djasper@google.com</a>></span> wrote:<br>
</div><div class="gmail_extra"><div class="gmail_quote"><div class="im">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><br>
<br>
================<br>
Comment at: lib/ASTMatchers/ASTMatchFinder.cpp:425<br>
@@ -423,3 +424,3 @@<br>
<div> private:<br>
-  bool matchesAncestorOfRecursively(<br>
+  bool memoizedMatchesAncestorOfRecursively(<br>
       const ast_type_traits::DynTypedNode &Node, const DynTypedMatcher &Matcher,<br>
</div>----------------<br>
I think the combination of these two methods is highly confusing. The name suggests that both do the same thing, just one with memoization and one without memoization, which they don't. Furthermore, the responsibilities are not clear at all. E.g. what happens if I call matchesAncestorOfRecursively() with the TUDecl? It will probably assert, as the check for that is only in memoizedMatchesAncestorOfRecursively()..<br>

</blockquote><div><br></div></div><div>Any constructive ideas on how to change the layout?</div></div></div></div></blockquote><div><br></div><div style>Reading them a few more times, I don't find them so bad anymore. I think some of my confusion actually stems from the BFS/DFS combination, so lets solve that first. Also, I'd probably put the TUDecl-check into the other method. Basically, I would think that matchesAncestorOfRecursively() should work if it calls itself recursively (without the memoized..) variant.</div>
<div style><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra">
<div class="gmail_quote"><div class="im"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Also, it seems a bit strange, to first match all direct parents (kind of BFS) and then recursively call matchesAncestorOfRecursively() on each one (kind of DFS). Is this intended?<br></blockquote><div><br></div></div><div>

Yes. That doesn't mean it was a good idea though :)</div></div></div></div>
</blockquote></div><br></div><div class="gmail_extra" style>Why not just do DFS? I.e.:</div><div class="gmail_extra" style><br></div><div class="gmail_extra" style><div class="gmail_extra">    for (ASTContext::ParentVector::const_iterator AncestorI = Parents.begin(),</div>
<div class="gmail_extra">                                                  AncestorE = Parents.end();</div><div class="gmail_extra">         AncestorI != AncestorE; ++AncestorI) {</div><div class="gmail_extra">      if (Matcher.matches(*AncestorI, this, Builder) || (MatchMode == ASTMatchFinder::AMM_ParentOnly && <span style="color:rgb(80,0,80)">memoizedMatchesAncestorOfRecur</span><span style="color:rgb(80,0,80)">sively(...))</span>)</div>
<div class="gmail_extra">        return true;</div><div class="gmail_extra">    }</div><div class="gmail_extra"><br></div><div class="gmail_extra" style>And then just remove the second loop?</div></div></div>