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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

The implementation looks good. Some comments around the test.



================
Comment at: clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp:161
 
+TEST_F(FindHeadersTest, TargetIsArgumentExpandedFromMacroInHeader) {
+  llvm::Annotations MainFile(R"cpp(
----------------
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)


================
Comment at: clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp:167
+  Inputs.Code = MainFile.code();
+  Inputs.ExtraFiles["declare.h"] = R"cpp(
+    #include "macro.h"
----------------
nit: use `guard(R"cpp(...)cpp)` for the normal header code.


================
Comment at: clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp:186
+  std::vector<Header> Headers;
+  walkUsed(
+      TopLevelDecls, {}, nullptr, AST->sourceManager(),
----------------
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;
      }
    };
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139716