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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchersInternal.h:88
+/// can be useful for cases like debugging matchers.
+template <typename T> std::string makeMatcherNameFromType() {
+  return "Matcher<T>";
----------------
ymandel wrote:
> Please note in the comments that this function template is specialized below to cover most (all) of the AST. So, the default string here should only very rarely (never?) be used.
Should we be asserting that we never use it so that we can have 100% coverage up front and rely on the assertion to catch later issues?


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchersInternal.h:90
+/// here should rarely be used.
+template <typename T> std::string makeMatcherNameFromType() {
+  return "Matcher<T>";
----------------
Does the return type need to be `std::string` instead of `StringRef`? It looks like everything is returning a string literal.


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchersInternal.h:578
+  /// If the name is already set, then \c MatcherName overrides it.
+  DynTypedMatcher withMatcherName(std::string MatcherName) const;
+
----------------
Is there a need for this to accept a `std::string` as opposed to `StringRef`?


================
Comment at: clang/lib/ASTMatchers/ASTMatchersInternal.cpp:131
 private:
+  const std::string MatcherName;
   std::vector<DynTypedMatcher> InnerMatchers;
----------------
FWIW, this is the only place I was expecting to see a std::string (places where it's needed for long-term storage).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113917