[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/ASTMatchers.h:67
@@ -66,3 +66,3 @@
 public:
   /// \brief Returns the AST node bound to \c ID.
   /// Returns NULL if there was no node bound to \c ID or if there is a node but
----------------
Daniel Jasper wrote:
> Blank line after \brief unless this is intentional.
Done.

================
Comment at: include/clang/ASTMatchers/ASTTypeTraits.h:28
@@ +27,3 @@
+/// Stores an AST node in a type safe way. This allows writing code that
+/// works with different kinds AST nodes, despite the fact that they don't
+/// have a common base class.
----------------
Daniel Jasper wrote:
> .. kinds of AST nodes ..
Done.

================
Comment at: include/clang/ASTMatchers/ASTTypeTraits.h:51
@@ +50,3 @@
+  /// For types that have identity via their pointer in the AST
+  /// (like Stmt and Decl) the returned pointer points to the
+  /// referenced AST node.
----------------
Daniel Jasper wrote:
> maybe: (like \c Stmt and \c Decl)
Done.

================
Comment at: include/clang/ASTMatchers/ASTTypeTraits.h:53
@@ +52,3 @@
+  /// referenced AST node.
+  /// For other types (like QualType) the value is stored directly
+  /// in the DynTypedNode, and the returned pointer points at
----------------
Daniel Jasper wrote:
> \c QualType here and \c DynTypedNode below
Done.

================
Comment at: include/clang/ASTMatchers/ASTTypeTraits.h:89
@@ +88,3 @@
+  /// Note that we store the QualType by value as we only get
+  /// it by-value from the AST.
+  llvm::AlignedCharArrayUnion<Decl*, Stmt*, QualType> Storage;
----------------
Daniel Jasper wrote:
> Some more documentation (or a reference) to what a QualType actually is (Type-pointer + 3Bits) might help understanding this easier.
I don't think we currently rely on that - for this implementation, all you need to know is that QualType doesn't have pointer identity and thus we need to store it by-value.

================
Comment at: include/clang/ASTMatchers/ASTTypeTraits.h:96
@@ -77,1 +95,3 @@
+    if (Tag == NT_Decl)
+      return dyn_cast<T>(*reinterpret_cast<Decl*const*>(Storage));
     return NULL;
----------------
Daniel Jasper wrote:
> I'd be consistent with getMemoizationData (spacing around * and using Storage.buffer).
> 
> Actually, how about:
> return dyn_cast<T>(getMemoizationData());
> ?
I dislike using getMemoizationData here. The other way around makes sense. 

Storage.buffer is different, because we get 'Storage' as a const char[] in this method. Perhaps I should rename it to Buffer to be more consistent?

================
Comment at: include/clang/ASTMatchers/ASTTypeTraits.h:110
@@ -88,1 +109,3 @@
+    if (Tag == NT_Stmt)
+      return dyn_cast<T>(*reinterpret_cast<Stmt*const*>(Storage));
     return NULL;
----------------
Daniel Jasper wrote:
> same as above
Same answer :P

================
Comment at: lib/ASTMatchers/ASTMatchFinder.cpp:36
@@ -33,3 +35,3 @@
 // identifying the matcher) and a pointer to the AST node.
 typedef std::pair<uint64_t, const void*> UntypedMatchInput;
 
----------------
Daniel Jasper wrote:
> Add a comment why we don't want memoization on QualTypes (e.g. might be incorrect together with the coming hasAncestor matchers or unnecessary as there are no big QualType hierarchies).
> 
> Also, maybe add FIXME to add memoization to for TypeLocs, once we match those (TypeLocs form bigger trees and presumably memoization is worth it). Also then it might make sense to store DynTypedNodes instead of void*!?
Tried to clarify in the comments.

Every TypeLoc has a QualType, so I don't see why the second argument holds - we'll either want to add memoization for both TypeLoc and QualType, or for neither.

Using DynTypedNode in the key doesn't make sense at all - we'll never want to get the original value back, we just need a one-way function that is unique.

================
Comment at: lib/ASTMatchers/ASTMatchFinder.cpp:260
@@ +259,3 @@
+    const UntypedMatchInput input(Matcher.getID(), Node.getMemoizationData());
+    assert(input.second &&
+           "Fix getMemoizationData once more types allow recursive matching.");
----------------
Daniel Jasper wrote:
> Why couldn't you reach this with a DynTypedNode representing a QualType? If you can, add test.
Because matchesDescendantOf (ASTMatchersInternal:444) only works for Decl and Stmt currently...

================
Comment at: lib/ASTMatchers/ASTMatchFinder.cpp:352
@@ -363,2 +351,3 @@
+                               MatchCallback*> >::const_iterator
              It = Triggers->begin(), End = Triggers->end();
          It != End; ++It) {
----------------
Daniel Jasper wrote:
> As you are here anyway, you could rename this to use the standard I and E, but up to you.
Done.


http://llvm-reviews.chandlerc.com/D33