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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

Looks good, just small 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>";
----------------
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.


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchersInternal.h:436
+
+  virtual std::string getName() const = 0;
 };
----------------
As a pure virtual method, this is defining an API. Please add a comment that explains to users and implementors any expecations/requirements on this method.


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchersInternal.h:1022
+  std::string getName() const override {
+    return "HasOverloadedOperatorNameMatcher";
+  }
----------------
here and below: drop the "Matcher" suffix?


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchersInternal.h:1375
+  auto MatcherName = makeMatcherNameFromType<T>();
   // For the size() == 0 case, we return a "true" matcher.
   if (InnerMatchers.empty()) {
----------------
here and line 1381: maybe replace "return" with "wrap"? The existing comment is wrong as is and feels even more off with this change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113917