[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.

[Manuel Klimek]

================
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