[PATCH] D80623: WIP: Add an API to simplify setting TraversalKind in clang-tidy matchers

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks for putting this together!
As mentioned on the cfe-dev thread, for our out-of-tree checks we're planning to wrap with traverse() instead, to capture the full matcher logic more locally at the cost of some readability.
So if this won't be used for in-tree checks, we'd probably want to hold off on this unless someone in particular plans to use it.



================
Comment at: clang/include/clang/ASTMatchers/ASTMatchFinder.h:118
+    /// behavior of clang-tidy.
+    virtual llvm::Optional<ast_type_traits::TraversalKind>
+    getCheckTraversalKind() const;
----------------
I don't really get why this would be optional.
A check's implementation isn't really going to be robust to "whatever the default is", it's going to be tested against one or the other.
So None isn't really a sensible return value - can't the base class simply return the actual default?


================
Comment at: clang/lib/ASTMatchers/ASTMatchFinder.cpp:1063
+  if (auto TK = Action->getCheckTraversalKind()) {
+    llvm::errs() << "Overriding getCheckTraversalKind is deprecated. Port to "
+                    "traverse() matcher instead.";
----------------
Writing to stderr from a library isn't ideal. (e.g. in multithreaded programs - clangd uses a wrapped stderr as its log stream, this bypasses the mutex)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80623