[PATCH] D139716: [include-cleaner] Use expansion locations for macros.

[Viktoriia Bakalova via Phabricator via cfe-commits]

VitaNuo marked an inline comment as done.
VitaNuo added inline comments.


================
Comment at: clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp:161
 
+TEST_F(FindHeadersTest, TargetIsArgumentExpandedFromMacroInHeader) {
+  llvm::Annotations MainFile(R"cpp(
----------------
hokein wrote:
> these 3 newly-added tests are similar, instead of duplicating them, we can use a table-gen style test, see an [example](https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp#L265)
Great idea, thanks!


================
Comment at: clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp:186
+  std::vector<Header> Headers;
+  walkUsed(
+      TopLevelDecls, {}, nullptr, AST->sourceManager(),
----------------
hokein wrote:
> The intention of the test is to test the function `findHeaders`, we're testing it in an implicit way (`findHeaders` is hidden inside the `walkUsed`), there is no code here explicitly calling this function. 
> 
> We can make it clearer (no need to use walkUsed):
> 1. we get a `Foo` Decl AST node from the testcase.
> 2. with the AST node from step1, we cam call the `findHeaders` directly in the test code.
> 
> for 1, we can just write a simple RecursiveASTVisitor, and capture a NamedDecl which called Foo, something like:
> 
> ```
>  struct Visitor : RecursiveASTVisitor<Visitor> {
>       const NamedDecl *Out = nullptr;
>       bool VisitNamedDecl(const NamedDecl *ND) {
>         if (ND->getName() == "Foo") {
>           EXPECT_TRUE(Out == nullptr);
>           Out = ND;
>         }
>         return true;
>       }
>     };
> ```
Ok, I've attempted to follow your advice now, please have a look.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139716/new/

https://reviews.llvm.org/D139716