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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
