aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land.
LGTM! ================ 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); ---------------- steveire wrote: > 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. > > > > 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. Nope, just making sure I understood. > 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. Good to know! FWIW, it would be reasonable to do some of that work in this patch, but I don't insist. As with one of the other patches in the set, making the changes to handle either traversal kind as appropriate for the situation improves the test coverage for your changes and it helps demonstrate why this default is more simple (as opposed to making everything look more complex because the patch only adds code, doesn't remove any). > This patch maintains the status quo as much as possible while also making it > possible to change the default. That's good enough for me, thanks. 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
