[PATCH] D90982: Ignore implicit nodes in IgnoreUnlessSpelledInSource mode

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thank you for this, I think it's mostly looking good!



================
Comment at: clang/include/clang/AST/ASTNodeTraverser.h:88
   void Visit(const Decl *D) {
+    if (Traversal != TK_AsIs && D->isImplicit())
+      return;
----------------
Similar to the feedback on D90984, I think this will do the wrong thing for the weird traversal mode for ignoring parens and implicit casts. It should probably be checking for `Traversal == TK_IgnoreUnlessSpelledInSource` here.


================
Comment at: clang/include/clang/AST/ASTNodeTraverser.h:192
   void Visit(const CXXCtorInitializer *Init) {
+    if (Traversal != TK_AsIs && !Init->isWritten())
+      return;
----------------
Same issue here.


================
Comment at: clang/include/clang/AST/ASTNodeTraverser.h:410
 
+    if (Traversal != TK_AsIs && D->isDefaulted())
+      return;
----------------
Same here. I'll stop calling these out -- can you double check all the uses of `Traversal != TK_AsIs`?


================
Comment at: clang/include/clang/AST/ASTNodeTraverser.h:733
+  void VisitCXXForRangeStmt(const CXXForRangeStmt *Node) {
+    if (Traversal != TK_AsIs) {
+      Visit(Node->getInit());
----------------
If we're in AsIs mode, don't we still want to match on some parts of the range-based for loop because they are spelled in source (like the body of the loop or the range-expression, etc)? 

The test behavior looks reasonable around this, so I suspect I'm just misunderstanding this change.


================
Comment at: clang/include/clang/AST/ASTNodeTraverser.h:742
+  void VisitCallExpr(const CallExpr *Node) {
+    for (auto Child :
+         make_filter_range(Node->children(), [this](const Stmt *Child) {
----------------
This be `const auto *` or at least `auto *`.


================
Comment at: clang/lib/ASTMatchers/ASTMatchFinder.cpp:152
+
+    if (DeclNode && DeclNode->isImplicit() && !Finder->isTraversalAsIs())
+      return baseTraverse(*DeclNode);
----------------
Similar issue here about traversal mode.


================
Comment at: clang/lib/ASTMatchers/ASTMatchFinder.cpp:1083
+
+  auto ScopedTraversal =
+      TraversingASTNodeNotSpelledInSource || DeclNode->isImplicit();
----------------
Spelling this as `bool` would not be amiss (same below, given how trivial it is to spell the type out).


================
Comment at: clang/lib/ASTMatchers/ASTMatchFinder.cpp:1108
   }
+  auto ScopedTraversal = TraversingASTNodeNotSpelledInSource ||
+                         TraversingASTChildrenNotSpelledInSource;
----------------
`bool` here as well, please.


================
Comment at: clang/lib/ASTMatchers/ASTMatchFinder.cpp:1157
 
+  auto ScopedTraversal = TraversingASTNodeNotSpelledInSource ||
+                         TraversingASTChildrenNotSpelledInSource;
----------------
Here too.


================
Comment at: clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp:2503
+    EXPECT_TRUE(matches(Code, traverse(TK_IgnoreUnlessSpelledInSource, M)));
+  }
+
----------------
Can you add one more test for the body like `cxxForRangeStmt(hasBody(compoundStmt()))` which should match in both modes?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90982