[cfe-commits] [PATCH] Introduces DynTypedMatcher as a new concept that replaces the UntypedBaseMatcher and TypedMatcher. Due to DynTypedNode the basic dynamically typed matcher interface can now be simplified.

[Sean Silva]

> ================
> Comment at: lib/ASTMatchers/ASTMatchFinder.cpp:353-355
> @@ -367,4 +352,5 @@
> +                             this, &Builder)) {
>          BoundNodesTree BoundNodes = Builder.build();
>          MatchVisitor Visitor(ActiveASTContext, It->second);
>          BoundNodes.visitMatches(&Visitor);
>        }
> ----------------
> Sean Silva wrote:
>> I know this isn't directly related to this patch, but this is really the nerve-center of the matching process, and deserves to be better explained (or cross-referenced to where it is explained).
> Hm, which parts need more explanations? Anything specific?

It's not that it's that hard to understand, but that it's hard to
find. So, for example, I think it would be good to mention it in the
comment at the top of the file. Also, I think a comment on match()
should definitely say "this is the core of the matching process", or
something like that. In other words, in order to understand the code,
this is the best place to start at (AFAIK), and the reader should be
aware of that so that they don't waste their time.

If there is another place which is a better place to start, then that
should be the thing called out in the comment at the top of the file
of course.

--Sean Silva

On Mon, Sep 3, 2012 at 9:23 AM, Manuel Klimek
<reviews at llvm-reviews.chandlerc.com> wrote:
>
>
> ================
> Comment at: include/clang/ASTMatchers/ASTMatchFinder.h:132-133
> @@ -127,7 +131,4 @@
>  private:
> -  /// \brief The MatchCallback*'s will be called every time the
> -  /// UntypedBaseMatcher matches on the AST.
> -  std::vector< std::pair<
> -    const internal::UntypedBaseMatcher*,
> -    MatchCallback*> > Triggers;
> +  /// \brief The triggers executed against the AST.
> +  std::vector<Trigger> Triggers;
>
> ----------------
> Sean Silva wrote:
>> Manuel Klimek wrote:
>> > Sean Silva wrote:
>> > > Since "trigger" is just a plain pair, it's not really clear at all what "executed against the AST means" for a trigger without some supporting documentation. Also as I mentioned above, putting the definition of `Trigger` nearby would help too.
>> > >
>> > > (in general, a more self-documenting name than "trigger" would be good ("MatcherWithAction"?)).
>> > Yea, I didn't really like the name either, and the more I think about it, the worse "Trigger" sounds.
>> >
>> > How about MatchAction?
>> To me, MatchAction sounds like "the action corresponding to a match", which is what MatchCallback is.
> Ok, couldn't come up with a better name and reverted the pull-out of the abstraction for now. Instead did a tyepdef of MatchCallback in the .cpp file for brevity of the type.
>
> ================
> Comment at: lib/ASTMatchers/ASTMatchFinder.cpp:353-355
> @@ -367,4 +352,5 @@
> +                             this, &Builder)) {
>          BoundNodesTree BoundNodes = Builder.build();
>          MatchVisitor Visitor(ActiveASTContext, It->second);
>          BoundNodes.visitMatches(&Visitor);
>        }
> ----------------
> Sean Silva wrote:
>> I know this isn't directly related to this patch, but this is really the nerve-center of the matching process, and deserves to be better explained (or cross-referenced to where it is explained).
> Hm, which parts need more explanations? Anything specific?
>
> ================
> Comment at: lib/ASTMatchers/ASTMatchFinder.cpp:257-258
> @@ -257,1 +256,4 @@
> +    const UntypedMatchInput input(Matcher.getID(), Node.getMemoizationData());
> +    assert(input.second &&
> +           "Fix getMemoizationData once more types allow recursive matching.");
>      std::pair<MemoizationMap::iterator, bool> InsertResult
> ----------------
> Sean Silva wrote:
>> Maybe put a `// FIXME` near this to keep things grep'able.
> There is actually no FIXME here - this code will not need to be adapted. The assert() is just there for the case where getMemoizationData() is out of sync with the rest of the DynTypedNode (which hopefully will not happen ;)
>
>
> http://llvm-reviews.chandlerc.com/D33