Details
- Reviewers
aaron.ballman
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Could you give me a bit more background about why you want to make this change?
There are tests which I would expect to match in any traversal mode (e.g., EXPECT_TRUE(matches("class X {};", traverse(TK_AsIs, HasClassX)));) so there's a part of me that wonders if we should add helper functionality to test the various traversal modes as part of this change. e.g., we specify an explicit traversal mode for the situations where only one mode should be tested, and we specify a helper function that tests both traversal modes on the same code. WDYT?
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. :-)
clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp | ||
---|---|---|
698 | Are you planning to answer this as part of this patch? | |
clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp | ||
3626 | 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). |
Are you planning to answer this as part of this patch?