[include-cleaner] Use expansion locations for macros.

[VitaNuo]

Summary

Use expansion locations for target symbol decls when finding headers for macros.

[VitaNuo]

Remove FIXME.

[hokein]

The implementation looks good. Some comments around the test.

clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp

162

these 3 newly-added tests are similar, instead of duplicating them, we can use a table-gen style test, see an example

168

nit: use guard(R"cpp(...)cpp) for the normal header code.

187

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;
     }
   };

[VitaNuo]

Address review comments.

[VitaNuo]

Add guard calls to headers.

[VitaNuo]

clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp

162

Great idea, thanks!

187

Ok, I've attempted to follow your advice now, please have a look.

[hokein]

thanks, the test looks better now, a few more suggestions.

clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp

162

Move the definition to the TEST_F body.

174

I think if (ND->getName() == "FLAG_foo" || ND->getName() == "Foo") should just work (without the VisitCXXRecordDecl method)?

190

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.

227

nit: Annotations is not needed as we don't use any annotation here.

233

Visitor.TraverseDecl(AST->context().getTranslationUnitDecl());

241

this is not needed -- the following EXPECT_THAT already covers it.

[VitaNuo]

Address most recent review comments.

[VitaNuo]

Thanks for the review!

[VitaNuo]

Remove extra variable.

[hokein]

Thanks!

clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp

176

nit: it feels more natural to declare MacroHeader first then DeclareHeader, as the DeclareHeader already includes the macro.h

177

The clang-format's indentation seems odd to me, I think if you put a trailing , on the last element }, };, its format is better.

178

nit: I'd add /*DeclareHeader=*/, /*MacroHeader=*/, which improves the code readability.

212

nit: /*PragmaIncludes=*/nullptr

[VitaNuo]

Address review comments.

[VitaNuo]

Thanks!

clang-tools-extra/include-cleaner/lib/FindHeaders.cpp

 namespace clang::include_cleaner {
 
 llvm::SmallVector<Header> findHeaders(const SymbolLocation &Loc,
                                       const SourceManager &SM,
                                       const PragmaIncludes *PI) {
   llvm::SmallVector<Header> Results;
   switch (Loc.kind()) {
   case SymbolLocation::Physical: {
-    // FIXME: Handle macro locations.
-    FileID FID = SM.getFileID(Loc.physical());
+    FileID FID = SM.getFileID(SM.getExpansionLoc(Loc.physical()));
     const FileEntry *FE = SM.getFileEntryForID(FID);
     if (!PI) {
       return FE ? llvm::SmallVector<Header>{Header(FE)}
                 : llvm::SmallVector<Header>();
     }
     while (FE) {
       Results.push_back(Header(FE));
       // FIXME: compute transitive exporter headers.

clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp

 //===--- FindHeadersTest.cpp ----------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "AnalysisInternal.h"
+#include "clang-include-cleaner/Analysis.h"
 #include "clang-include-cleaner/Record.h"
+#include "clang-include-cleaner/Types.h"
+#include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Basic/FileEntry.h"
 #include "clang/Basic/FileManager.h"
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Testing/TestAST.h"
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/Support/raw_ostream.h"
+#include "llvm/Testing/Support/Annotations.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include <memory>
 
 namespace clang::include_cleaner {
 namespace {
 using testing::UnorderedElementsAre;
 
 std::string guard(llvm::StringRef Code) {
   return "#pragma once\n" + Code.str();
 }
 
 class FindHeadersTest : public testing::Test {
 protected:
   TestInputs Inputs;
   PragmaIncludes PI;
   std::unique_ptr<TestAST> AST;
   FindHeadersTest() {
     Inputs.MakeAction = [this] {
-      struct Hook : public PreprocessOnlyAction {
+      struct Hook : public SyntaxOnlyAction {
       public:
         Hook(PragmaIncludes *Out) : Out(Out) {}
         bool BeginSourceFileAction(clang::CompilerInstance &CI) override {
           Out->record(CI);
           return true;
         }
 
         PragmaIncludes *Out;
TEST_F(FindHeadersTest, NonSelfContainedTraverseExporter) {
   buildAST();
   // Verify that we emit exporters for each header on the path.
   EXPECT_THAT(findHeaders("fragment.inc"),
               UnorderedElementsAre(physicalHeader("fragment.inc"),
                                    physicalHeader("exported.h"),
                                    physicalHeader("exporter.h")));
 }
 
+TEST_F(FindHeadersTest, TargetIsExpandedFromMacroInHeader) {
+  struct CustomVisitor : RecursiveASTVisitor<CustomVisitor> {
+    const Decl *Out = nullptr;
+    bool VisitNamedDecl(const NamedDecl *ND) {
+      if (ND->getName() == "FLAG_foo" || ND->getName() == "Foo") {
+        EXPECT_TRUE(Out == nullptr);
+        Out = ND;
+      }
+      return true;
+    }
+  };
+
+  struct {
+    llvm::StringRef MacroHeader;
+    llvm::StringRef DeclareHeader;
+  } TestCases[] = {
+      {/*MacroHeader=*/R"cpp(
+    #define DEFINE_CLASS(name) class name {};
+  )cpp",
+       /*DeclareHeader=*/R"cpp(
+    #include "macro.h"
+    DEFINE_CLASS(Foo)
+  )cpp"},
+      {/*MacroHeader=*/R"cpp(
+    #define DEFINE_Foo class Foo {};
+  )cpp",
+       /*DeclareHeader=*/R"cpp(
+    #include "macro.h"
+    DEFINE_Foo
+  )cpp"},
+      {/*MacroHeader=*/R"cpp(
+    #define DECLARE_FLAGS(name) extern int FLAG_##name
+  )cpp",
+       /*DeclareHeader=*/R"cpp(
+    #include "macro.h"
+    DECLARE_FLAGS(foo);
+  )cpp"},
+  };
+
+  for (const auto &T : TestCases) {
+    Inputs.Code = R"cpp(#include "declare.h")cpp";
+    Inputs.ExtraFiles["declare.h"] = guard(T.DeclareHeader);
+    Inputs.ExtraFiles["macro.h"] = guard(T.MacroHeader);
+    buildAST();
+
+    CustomVisitor Visitor;
+    Visitor.TraverseDecl(AST->context().getTranslationUnitDecl());
+
+    llvm::SmallVector<Header> Headers = clang::include_cleaner::findHeaders(
+        Visitor.Out->getLocation(), AST->sourceManager(),
+        /*PragmaIncludes=*/nullptr);
+    EXPECT_THAT(Headers, UnorderedElementsAre(physicalHeader("declare.h")));
+  }
+}
+
 } // namespace
 } // namespace clang::include_cleaner