hokein added a comment.
thanks, the test looks better now, a few more suggestions.
================
Comment at: clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp:162
+struct CustomVisitor : RecursiveASTVisitor<CustomVisitor> {
+ const Decl *Out = nullptr;
----------------
Move the definition to the `TEST_F` body.
================
Comment at: clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp:174
+ bool VisitNamedDecl(const NamedDecl *ND) {
+ if (ND->getName() == "FLAG_foo") {
+ EXPECT_TRUE(Out == nullptr);
----------------
I think `if (ND->getName() == "FLAG_foo" || ND->getName() == "Foo")` should
just work (without the `VisitCXXRecordDecl` method)?
================
Comment at: clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp:190
+ #include "declare.h"
+ Foo foo;
+ )cpp",
----------------
I think the `Foo foo;` is not really needed for the test (`#include
"declare.h"` is the key part of the main file) as we only want the location of
the `CXXRecordDecl`. The same for the following testcases.
So I think we can simplify it further: TestCases has two members
(`DeclareHeader`, `MacroHeader`). The main file content is always `#include
"declare.h"`, we can construct the main code in the for-loop body.
================
Comment at: clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp:227
+ for (const auto &T : TestCases) {
+ llvm::Annotations MainFile(T.Main);
+ Inputs.Code = MainFile.code();
----------------
nit: Annotations is not needed as we don't use any annotation here.
================
Comment at: clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp:233
+
+ CustomVisitor Visitor;
+ for (Decl *D : AST->context().getTranslationUnitDecl()->decls()) {
----------------
Visitor.TraverseDecl(AST->context().getTranslationUnitDecl());
================
Comment at: clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp:241
+ Foo->getLocation(), AST->sourceManager(), nullptr);
+ EXPECT_EQ(Headers.size(), 1u);
+ EXPECT_THAT(Headers, UnorderedElementsAre(physicalHeader("declare.h")));
----------------
this is not needed -- the following `EXPECT_THAT` already covers it.
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
