> ================
> 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
<[email protected]> 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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
