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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
