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

[Daniel Jasper]

================
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;
 
----------------
Manuel Klimek wrote:
> 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.
Yes, exactly, every TypeLoc has a QualType, so it is sufficient to memoize the TypeLoc (memoizing the QualTypes, too, would at best lead to an improvement by a small constant factor). However, TypeLocs can form longer chains and we don't want to match whole subchains repeatedly for hasDescendant()..

"We just need a one-way function that is unique".. This is sort of what DynTypedNode provides (it already knows that for Decls and Stmts the pointer is unique and for QualTypes it has to use the value). Would be good to store information like that in only one class (even though we will never need the retrieval-logic). On the other hand, if you disagree on the first point and say that we will only ever want to memoize Stmts and Decls, this is a mute point ;-).

================
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;
----------------
Manuel Klimek wrote:
> 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?
Yes, the other way around makes more sense. I definitely like having only one reinterpret_cast :-).

'Storage' is fine, I just did not read carefully enough, sorry!

================
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;
----------------
Manuel Klimek wrote:
> 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.
.. which I would find easier to understand than what the comment currently says.

How about:

/// Note that we can store \c Decls and \c Stmts by pointer as they are
/// guaranteed be unique pointers pointing to dedicated storage in the
/// AST. \c QualTypes on the other hand do not have storage or unique
/// pointers and thus need to be stored by value.
/// Further info: <reference to QualType>

(with the "Further info" completely optional).


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