[PATCH] D61837: Make it possible control matcher traversal kind with ASTContext

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thanks for this -- it looks like really interesting functionality! I've mostly found nits thus far, but did have a question about clang-query support for it.



================
Comment at: include/clang/AST/ASTContext.h:562-563
 public:
+  ast_type_traits::TraversalKind GetTraversalKind() const { return Traversal; }
+  void SetTraversalKind(ast_type_traits::TraversalKind TK) { Traversal = TK; }
+
----------------
This doesn't match our coding guidelines. Should be `getTraversalKind()`, etc. Same below.


================
Comment at: include/clang/AST/ASTContext.h:565-568
+  const Expr *TraverseIgnored(const Expr *E);
+  Expr *TraverseIgnored(Expr *E);
+  ast_type_traits::DynTypedNode
+  TraverseIgnored(const ast_type_traits::DynTypedNode &N);
----------------
Is there a reason we don't want these functions to be marked `const`?


================
Comment at: include/clang/AST/ASTNodeTraverser.h:80
 
+  void SetTraversalKind(ast_type_traits::TraversalKind TK) { Traversal = TK; }
+
----------------
`setTraversalKind()`


================
Comment at: include/clang/AST/ASTNodeTraverser.h:105
 
-  void Visit(const Stmt *S, StringRef Label = {}) {
+  void Visit(const Stmt *S_, StringRef Label = {}) {
     getNodeDelegate().AddChild(Label, [=] {
----------------
Let's not make underscores important like this -- how about `Node` or something generic instead?


================
Comment at: include/clang/AST/ASTNodeTraverser.h:113-114
+          break;
+        case ast_type_traits::TK_IgnoreImplicitCastsAndParentheses:
+          S = E->IgnoreParenImpCasts();
+          break;
----------------
What an unfortunate name for this. `IgnoreParenImpCasts()` ignores parens, implicit casts, full expressions, temporary materialization, and non-type template substitutions, and those extra nodes have surprised people in the past.

Not much to be done about it here, just loudly lamenting the existing name of the trait.


================
Comment at: include/clang/ASTMatchers/ASTMatchers.h:701
 
+/// Causes all nested matchers to be matched with the specified traversal kind
+///
----------------
Missing a full stop at the end of the comment.


================
Comment at: include/clang/ASTMatchers/ASTMatchers.h:716
+/// \endcode
+/// matches the return statement with "ret" bound to "a".
+template <typename T>
----------------
Copy pasta?


================
Comment at: include/clang/ASTMatchers/ASTMatchers.h:718
+template <typename T>
+internal::Matcher<T> traverse(ast_type_traits::TraversalKind TK,
+                              const internal::Matcher<T> &InnerMatcher) {
----------------
Is this an API we should be exposing to clang-query as well? Will those users be able to use a string literal for the traversal kind, like they already do for attribute kinds (etc)?


================
Comment at: include/clang/ASTMatchers/ASTMatchersInternal.h:286
+
+  virtual llvm::Optional<ast_type_traits::TraversalKind> TraversalKind() const {
+    return {};
----------------
`traversalKind()`


================
Comment at: include/clang/ASTMatchers/ASTMatchersInternal.h:287
+  virtual llvm::Optional<ast_type_traits::TraversalKind> TraversalKind() const {
+    return {};
+  }
----------------
`return llvm::None;`


================
Comment at: lib/AST/ASTContext.cpp:120
+ASTContext::TraverseIgnored(const ast_type_traits::DynTypedNode &N) {
+  if (auto E = N.get<Expr>()) {
+    return ast_type_traits::DynTypedNode::create(*TraverseIgnored(E));
----------------
`auto *`


================
Comment at: lib/AST/ASTContext.cpp:10358
   bool TraverseStmt(Stmt *StmtNode) {
+    auto FilteredNode = StmtNode;
+    if (auto *ExprNode = dyn_cast_or_null<Expr>(FilteredNode))
----------------
`Stmt *`


================
Comment at: lib/ASTMatchers/ASTMatchFinder.cpp:148
     Stmt *StmtToTraverse = StmtNode;
+    if (Expr *ExprNode = dyn_cast_or_null<Expr>(StmtNode))
+      StmtToTraverse = Finder->getASTContext().TraverseIgnored(ExprNode);
----------------
`auto *`


================
Comment at: lib/ASTMatchers/ASTMatchersInternal.cpp:214-215
                               BoundNodesTreeBuilder *Builder) const {
-  if (RestrictKind.isBaseOf(DynNode.getNodeKind()) &&
-      Implementation->dynMatches(DynNode, Finder, Builder)) {
+  auto PreviousTraversalKind = Finder->getASTContext().GetTraversalKind();
+  auto OptTK = Implementation->TraversalKind();
+  if (OptTK)
----------------
Please do not use `auto` here.


================
Comment at: lib/ASTMatchers/ASTMatchersInternal.cpp:218-219
+    Finder->getASTContext().SetTraversalKind(*OptTK);
+  auto N = Finder->getASTContext().TraverseIgnored(DynNode);
+  auto NodeKind = N.getNodeKind();
+
----------------
Or here...


================
Comment at: lib/ASTMatchers/ASTMatchersInternal.cpp:237-238
     BoundNodesTreeBuilder *Builder) const {
-  assert(RestrictKind.isBaseOf(DynNode.getNodeKind()));
-  if (Implementation->dynMatches(DynNode, Finder, Builder)) {
+  auto N = Finder->getASTContext().TraverseIgnored(DynNode);
+  auto NodeKind = N.getNodeKind();
+
----------------
Or here...


================
Comment at: lib/ASTMatchers/ASTMatchersInternal.cpp:240
+
+  assert(RestrictKind.isBaseOf(NodeKind));
+  if (Implementation->dynMatches(N, Finder, Builder)) {
----------------
Add an assertion message?


Repository:
  rC Clang

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

https://reviews.llvm.org/D61837