[cfe-commits] [PATCH] Matchers for NestedNameSpecifiers

[Manuel Klimek]

  First round of comments...


================
Comment at: include/clang/ASTMatchers/ASTMatchers.h:2315
@@ +2314,3 @@
+/// \brief Matches nested name specifiers that specify a type matching the
+/// given \c QualType matcher with no qualifiers.
+/// FIXME: This is a temporary solution. Switch to using Type-matchers as soon
----------------
s/with no/without/

================
Comment at: include/clang/ASTMatchers/ASTMatchersInternal.h:733
@@ -733,1 +732,3 @@
 
+/// \brief The \c LocExtractor has a static method extract that can extract the
+/// underlying object of type \c T out of a \c TLoc object.
----------------
insert "to"

================
Comment at: include/clang/ASTMatchers/ASTMatchersInternal.h:739
@@ +738,3 @@
+template <typename TLoc, typename T> struct LocExtractor;
+template<> struct LocExtractor<NestedNameSpecifierLoc, NestedNameSpecifier> {
+  static const NestedNameSpecifier *extract(const NestedNameSpecifierLoc &Loc) {
----------------
Any reasons not to put this as overloaded methods into LocMatcher?

================
Comment at: include/clang/ASTMatchers/ASTMatchersInternal.h:825
@@ +824,3 @@
+    return BindableMatcher<T>(new TrueMatcher<T>);
+  Matcher<T> InnerMatcher = makeMatcher(new TrueMatcher<T>);
+  for (int i = InnerMatchers.size() - 1; i > 0; --i) {
----------------
I'd write something like:
Matcher<T> *InnerMatcher = new TrueMatcher<T>;
for (int i = InnerMatchers.size() - 1; i > 0; --i) {	
    InnerMatcher = new AllOfMatcher<T, Matcher<T>, Matcher<T> >(	
      *InnerMatchers[i], makeMatcher(InnerMatcher));	
  }	
  return BindableMatcher<T>(InnerMatcher);

================
Comment at: include/clang/ASTMatchers/ASTTypeTraits.h:88
@@ -85,3 +87,3 @@
   /// pointers and thus need to be stored by value.
-  llvm::AlignedCharArrayUnion<Decl*, Stmt*, QualType> Storage;
+  llvm::AlignedCharArrayUnion<Decl*, Stmt*, NestedNameSpecifierLoc, QualType> Storage;
 };
----------------
80 columns

================
Comment at: include/clang/ASTMatchers/ASTTypeTraits.h:132
@@ +131,3 @@
+template<> struct DynTypedNode::BaseConverter<NestedNameSpecifierLoc, void> {
+  static const NestedNameSpecifierLoc *get(NodeTypeTag Tag, const char Storage[]) {
+    if (Tag == NT_NestedNameSpecifierLoc)
----------------
80 columns

================
Comment at: lib/ASTMatchers/ASTMatchFinder.cpp:569
@@ +568,3 @@
+  match(NNS);
+  match(*NNS.getNestedNameSpecifier());
+  return
----------------
Please add a comment explaining that the traversal is already done through the "parallel hierarchy" in the locs, and we only want to match the nns themselves for each loc.

================
Comment at: include/clang/ASTMatchers/ASTMatchersInternal.h:744
@@ +743,3 @@
+};
+template<> struct LocExtractor<TypeLoc, Type> {
+  static const Type *extract(const TypeLoc &Loc) {
----------------
I'm opposed to adding in code that's not used :)


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