[cfe-commits] [PATCH] Matchers for Types, QualTypes and TypeLocs

[Manuel Klimek]

================
Comment at: include/clang/ASTMatchers/ASTMatchers.h:112
@@ -112,1 +111,3 @@
 typedef internal::Matcher<Stmt> StatementMatcher;
+typedef internal::Matcher<QualType> TypeMatcher;
+typedef internal::Matcher<TypeLoc> TypeLocMatcher;
----------------
Do we want to keep that backwards compatible, or do we want to rename this to QualTypeMatcher?

================
Comment at: include/clang/ASTMatchers/ASTMatchers.h:2463
@@ +2462,3 @@
+/// For details, see the documentation for the corresonding AST nodes.
+/// @{
+AST_TYPE_MATCHER(BuiltinType, builtinType);
----------------
Fix...

================
Comment at: include/clang/ASTMatchers/ASTMatchers.h:2492
@@ +2491,3 @@
+/// arrayType(hasElementType(builtinType()))
+///   matches "int b[7]"
+AST_TYPELOC_TRAVERSE_MATCHER(hasElementType, getElement);
----------------
For those we'll need to specify the matchers that it can act as, like this:
/// Usable as: Matcher<T1>, Matcher<T2>

================
Comment at: include/clang/ASTMatchers/ASTMatchers.h:2617
@@ +2616,3 @@
+/// memberPointerType()
+///   matches ""
+AST_TYPE_MATCHER(MemberPointerType, memberPointerType);
----------------
Missing something? :)

================
Comment at: include/clang/ASTMatchers/ASTMatchers.h:2712
@@ -2486,1 +2711,3 @@
 }
+AST_MATCHER_P(NestedNameSpecifierLoc, specifiesTypeLoc,
+              internal::Matcher<TypeLoc>, InnerMatcher) {
----------------
Doxy-comment?

================
Comment at: include/clang/ASTMatchers/ASTMatchers.h:2727
@@ -2497,2 +2726,3 @@
 /// nestedNameSpecifierLoc(hasPrefix(loc(specifiesType(asString("struct A")))))
 ///   both match "A::"
+inline internal::Matcher<NestedNameSpecifier> hasPrefix(
----------------
Make that a @{ comment for both, or have 2 comments.

================
Comment at: include/clang/ASTMatchers/ASTMatchersInternal.h:313
@@ +312,3 @@
+   public:
+    TypeToQualType(const Matcher<TypeT> &InnerMatcher, bool Strict)
+        : InnerMatcher(InnerMatcher), Strict(Strict) {}
----------------
I think we'll want to fix the strict in either direction, and not have two variants.

================
Comment at: include/clang/ASTMatchers/ASTMatchersInternal.h:1067
@@ -1032,1 +1066,3 @@
 
+class NNSPrefixMatcher : public MatcherInterface<NestedNameSpecifier> {
+public:
----------------
s/NNS/NestedNameSpecifier/

Also, please add a small doxy comment.

================
Comment at: include/clang/ASTMatchers/ASTMatchersInternal.h:1095
@@ +1094,3 @@
+    NestedNameSpecifierLoc NextNode = Node.getPrefix();
+    if (!NextNode)
+      return false;
----------------
I think we should be consistent with == NULL vs. implicit in this file.

================
Comment at: include/clang/ASTMatchers/ASTMatchersInternal.h:1174
@@ +1173,3 @@
+template <typename T>
+T makeTypeAllOfComposite(ArrayRef<const Matcher<QualType> *> InnerMatchers) {
+  MatcherInterface<QualType> *InnerMatcher = new TrueMatcher<QualType>();
----------------
if we need this, I'd expect the implementation to look like:
return T(makeAllOfComposite(InnerMatchers));


================
Comment at: include/clang/ASTMatchers/ASTMatchersInternal.h:1185
@@ +1184,3 @@
+template <typename T>
+T makeTypeLocAllOfComposite(ArrayRef<const Matcher<TypeLoc> *> InnerMatchers) {
+  MatcherInterface<TypeLoc> *InnerMatcher = new TrueMatcher<TypeLoc>();
----------------
Same here.

================
Comment at: include/clang/ASTMatchers/ASTMatchersMacros.h:227
@@ +226,3 @@
+  const internal::VariadicDynCastAllOfMatcher<TypeLoc,                         \
+                                              NodeType##Loc> MatcherName##Loc
+
----------------
One big question is whether we'd rather just write those...

================
Comment at: include/clang/ASTMatchers/ASTMatchersMacros.h:224
@@ -223,2 +223,3 @@
 
-/// \brief LOC_TRAVERSE_MATCHER(MatcherName, NodeType, FunctionName)
+#define AST_TYPE_MATCHER(NodeType, MatcherName)                                \
+  const internal::VariadicDynCastAllOfMatcher<Type, NodeType> MatcherName;     \
----------------
Comment.

================
Comment at: include/clang/ASTMatchers/ASTMatchersMacros.h:233
@@ -227,8 +232,3 @@
 ///
-/// The traversal is done using the given \c FunctionName.
-#define LOC_TRAVERSE_MATCHER(                                                  \
-      MatcherName, NodeType, FunctionName)                                     \
-  inline internal::Matcher<NodeType> hasPrefix(                                \
-      const internal::Matcher<NodeType> &InnerMatcher) {                       \
-    return internal::makeMatcher(new internal::TraverseMatcher<NodeType>(      \
-      InnerMatcher, &NodeType::getPrefix));                                    \
+/// The traversal is done using \c FunctionName.
+#define AST_TYPE_TRAVERSE_MATCHER(MatcherName, FunctionName)                   \
----------------
Perhaps: The traversal is done using \c NodeTypel::FunctionName.?

================
Comment at: include/clang/ASTMatchers/ASTMatchersMacros.h:256
@@ +255,3 @@
+};                                                                             \
+const Variadic##MatcherName##TypeTraverseMatcher MatcherName
+
----------------
I think we might be able to do this differently, but since this is an implementation detail, I'm fine with playing around with this later.

================
Comment at: include/clang/ASTMatchers/ASTTypeTraits.h:134
@@ -118,1 +133,3 @@
+  }
+};
 template<> struct DynTypedNode::BaseConverter<NestedNameSpecifier, void> {
----------------
Now that we have 3 cases, it makes sense to have an abstraction.

================
Comment at: lib/ASTMatchers/ASTMatchFinder.cpp:558
@@ +557,3 @@
+  match(TypeLocNode);
+  match(TypeLocNode.getType());
+  return RecursiveASTVisitor<MatchASTVisitor>::TraverseTypeLoc(TypeLocNode);
----------------
I think this warrants a comment that explains what's going on.


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