[PATCH] D90982: Ignore implicit nodes in IgnoreUnlessSpelledInSource mode
Aaron Ballman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Nov 16 12:04:38 PST 2020
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
More information about the cfe-commits
mailing list