steveire marked an inline comment as done. steveire added inline comments.
================ Comment at: clang/unittests/Tooling/StencilTest.cpp:63 ASTContext &Context = AstUnit->getASTContext(); - auto Matches = ast_matchers::match(wrapMatcher(Matcher), Context); + auto Matches = ast_matchers::match( + traverse(ast_type_traits::TK_AsIs, wrapMatcher(Matcher)), Context); ---------------- aaron.ballman wrote: > steveire wrote: > > aaron.ballman wrote: > > > Was this change made because you didn't want to accept the traversal mode > > > as a parameter to `matchStmt` and force each caller to decide which mode > > > they use? Or is there some other reason why this change was needed? > > Yes, the test currently expects `AsIs`. If someone wanted to change that in > > the future, that is an issue for future work. > You say "the test" as though there's only one. There are numerous tests which > use `matchStmt`. Do *all* of the tests require `AsIs`? (I think that's an > example of what's causing some of the confusion @shafik and I had and I'm > trying to ensure his question gets answered and that I'm fully understanding > your changes.) From spot-checking, it looks like there are tests using > `matchStmt` that don't need `AsIs` traversal and that this sort of cleanup > work could happen in the future, but for now you're going with the easiest > approach instead of expanding the test coverage for ignoring implicit nodes; > is that correct? I said "the" test because there is one `StencilTest` class inside one `StencilTest.cpp`. Perhaps not a word you would use, but that's not something to get stuck on hopefully. Yes, it is possible that someone could make `matchStmt` take an explicit `TraversalKind`, but I didn't see the need to do that in this patch. This patch maintains the status quo as much as possible while also making it possible to change the default. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72531/new/ https://reviews.llvm.org/D72531 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
