aaron.ballman added a comment. In D93988#2477961 <https://reviews.llvm.org/D93988#2477961>, @steveire wrote:
> In D93988#2477602 <https://reviews.llvm.org/D93988#2477602>, @aaron.ballman > wrote: > >> Could you give me a bit more background about why you want to make this >> change? > > This change makes it explicit so that we can see which tests work in only one > mode, and if we want to change the default, this part is already compatible > with the change. > > But, this is not very important. > >> There are tests which I would expect to match in any traversal mode (e.g., >> `EXPECT_TRUE(matches("class X {};", traverse(TK_AsIs, HasClassX)));`) > > Given that > > HasClassX = recordDecl(has(recordDecl(hasName("X")))) > > which matches the implicit `RecordDecl` within the `RecordDecl`, it shouldn't > match in `Ignore...` mode. My eyes managed to miss that this was grabbing the injected declaration and that was the root cause of my confusion in a few places. Thanks for the clarification. :-) ================ Comment at: clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp:698 + + // TODO: No change? EXPECT_TRUE(matches( ---------------- Are you planning to answer this as part of this patch? ================ Comment at: clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp:3626 + EXPECT_TRUE(matchAndVerifyResultTrue( + "template <typename T> struct A { struct B {" + " void f() { if(true) {} }" ---------------- Rather than duplicate the code, it may be a bit cleaner to hoist the string literal into a local variable that's shared between the two test cases. Also, a comment pointing out how these tests differ wouldn't be amiss (it took me a bit to see that the second test is ensuring we ignore the injected class name). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93988/new/ https://reviews.llvm.org/D93988 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
