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

[Haojian Wu via Phabricator via cfe-commits]

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