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