[PATCH] D93988: [ASTMatchers] Make tests explicit about mode-dependence

[Aaron Ballman via Phabricator via cfe-commits]

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