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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchFinder.h:118
+    /// behavior of clang-tidy.
+    virtual llvm::Optional<ast_type_traits::TraversalKind>
+    getCheckTraversalKind() const;
----------------
steveire wrote:
> sammccall wrote:
> > 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?
> ASTMatchFinder doesn't know the ASTContext, so it can't access the default. That's why I made it an optional.
The fact that it's optional is a bit weird. Given that this is a temporary API, I see two approaches: hold our nose because the ugliness shouldn't be long-lived, or have the default be specified in two places with comments saying "don't forget to update over here if you change this value" so we can get rid of the `Optional<>` here. WDYT?


================
Comment at: clang/lib/ASTMatchers/ASTMatchFinder.cpp:1205
                              MatchCallback *Action) {
-  Matchers.DeclOrStmt.emplace_back(NodeMatch, Action);
+  if (auto TK = Action->getCheckTraversalKind()) {
+    Matchers.DeclOrStmt.emplace_back(traverse(*TK, NodeMatch), Action);
----------------
Elide the braces.


================
Comment at: clang/lib/ASTMatchers/ASTMatchFinder.cpp:1221
                              MatchCallback *Action) {
-  Matchers.DeclOrStmt.emplace_back(NodeMatch, Action);
+  if (auto TK = Action->getCheckTraversalKind()) {
+    Matchers.DeclOrStmt.emplace_back(traverse(*TK, NodeMatch), Action);
----------------
Elide the braces.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80623