[PATCH] Implements memoization for ancestor matching.

Daniel Jasper djasper at google.com
Mon Mar 11 09:05:56 PDT 2013


Did you reply in phabricator or just on this email?


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 && memoizedMatchesAncestorOfRecursively(...))
)
        return true;
    }

And then just remove the second loop?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130311/14e06a58/attachment.html>


More information about the cfe-commits mailing list