[PATCH] D113917: Add infrastructure to support matcher names.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

The approach looks fine in general, just some nits when reading through the patch.



================
Comment at: clang/include/clang/ASTMatchers/ASTMatchersInternal.h:152
+  }
+MAKE_MATCHER_NAME_FROM_TYPE(TemplateArgument)
+MAKE_MATCHER_NAME_FROM_TYPE(TemplateArgumentLoc)
----------------
These are types that are not covered in the above gen .inc files. I wonder is there a way to verify this list is complete? 


================
Comment at: clang/lib/ASTMatchers/ASTMatchersInternal.cpp:120
+                  std::vector<DynTypedMatcher> InnerMatchers)
+      : MatcherName(MatcherName), InnerMatchers(std::move(InnerMatchers)) {}
 
----------------
std::move(MatcherName), and elsewhere.


================
Comment at: clang/lib/ASTMatchers/ASTMatchersInternal.cpp:164
+public:
+  NameMatcherImpl(std::string _MatcherName,
+                  IntrusiveRefCntPtr<DynMatcherInterface> InnerMatcher)
----------------
_MatcherName => MatcherName


================
Comment at: clang/lib/ASTMatchers/ASTMatchersInternal.cpp:306
+  Copy.Implementation =
+      new NameMatcherImpl(MatcherName, std::move(Copy.Implementation));
+  return Copy;
----------------
std::move(MatcherName)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113917/new/

https://reviews.llvm.org/D113917