[PATCH] Implements memoization for ancestor matching.

[Manuel Klimek]

On Mon, Mar 11, 2013 at 9:05 AM, Daniel Jasper <djasper at google.com> wrote:

> Did you reply in phabricator or just on this email?
>

Just mail.


>
>
> On Mon, Mar 11, 2013 at 3:57 PM, Manuel Klimek <klimek at google.com> wrote:
>
>> On Mon, Mar 11, 2013 at 7:19 AM, Daniel Jasper <djasper at google.com>wrote:
>>
>>>
>>>
>>> ================
>>> Comment at: lib/ASTMatchers/ASTMatchFinder.cpp:425
>>> @@ -423,3 +424,3 @@
>>>  private:
>>> -  bool matchesAncestorOfRecursively(
>>> +  bool memoizedMatchesAncestorOfRecursively(
>>>        const ast_type_traits::DynTypedNode &Node, const DynTypedMatcher
>>> &Matcher,
>>> ----------------
>>> 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()..
>>>
>>
>> Any constructive ideas on how to change the layout?
>>
>
> 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.
>
>  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?
>>>
>>
>> Yes. That doesn't mean it was a good idea though :)
>>
>
> Why not just do DFS? I.e.:
>
>     for (ASTContext::ParentVector::const_iterator AncestorI =
> Parents.begin(),
>                                                   AncestorE =
> Parents.end();
>          AncestorI != AncestorE; ++AncestorI) {
>       if (Matcher.matches(*AncestorI, this, Builder) || (MatchMode ==
> ASTMatchFinder::AMM_ParentOnly && memoizedMatchesAncestorOfRecur
> sively(...)))
>         return true;
>     }
>
> And then just remove the second loop?
>

Now that I think about it, we probably really want bfs (I think the
confusion came because in the beginning this was not actually getting
multiple parents): hasAncestor returns the first matching node, and we want
that to be the closest matching node, as usually you'll want to say "give
me the innermost function definition I'm in". I think not using bfs thus is
a bug. Thx for pointing it out.

Cheers,
/Manuel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130311/e89a8f28/attachment.html>