[clang-include-cleaner] make SymbolLocation a real class, move FindHeaders

[sammccall]

Summary

• replace SymbolLocation std::variant with enum-exposing version similar to those in types.cpp. There's no appropriate implementation file, added LocateSymbol.cpp in anticipation of locateDecl/locateMacro.
• FindHeaders is not part of the public Analysis interface, so should not be implemented/tested there (just code organization)
• rename findIncludeHeaders->findHeaders to avoid confusion with Include concept

[tschuett]

You could:

static_assert(std::variant_size_v<....> == 2);

Kind kind() const { if (std::holds_alternative<SourceLocation>(Storage))  return Kind::Physical; return Kind::Standard; }

and Kind could become enum class Kind. => You would get rid of the order on the variant and the static_cast.

[sammccall]

In D137825#3921273, @tschuett wrote:

You could:

static_assert(std::variant_size_v<....> == 2);

Kind kind() const { if (std::holds_alternative<SourceLocation>(Storage))  return Kind::Physical; return Kind::Standard; }

and Kind could become enum class Kind. => You would get rid of the order on the variant and the static_cast.

Sure, but that's IMO less regular, harder to understand, and easier to get wrong when updating.

[kadircet]

thanks!

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

void walkUsed(llvm::ArrayRef<Decl *> ASTRoots,
               UsedSymbolCB CB) {
   tooling::stdlib::Recognizer Recognizer;
   for (auto *Root : ASTRoots) {
     auto &SM = Root->getASTContext().getSourceManager();
     walkAST(*Root, [&](SourceLocation Loc, NamedDecl &ND, RefType RT) {
       if (auto SS = Recognizer(&ND)) {
         // FIXME: Also report forward decls from main-file, so that the caller
         // can decide to insert/ignore a header.
-        return CB({Loc, Symbol(*SS), RT}, findIncludeHeaders(*SS, SM, PI));
+        return CB({Loc, Symbol(*SS), RT}, findHeaders(*SS, SM, PI));
       }
       // FIXME: Extract locations from redecls.
-      return CB({Loc, Symbol(ND), RT},
-                findIncludeHeaders(ND.getLocation(), SM, PI));
+      return CB({Loc, Symbol(ND), RT}, findHeaders(ND.getLocation(), SM, PI));
     });
   }
   for (const SymbolReference &MacroRef : MacroRefs) {
     assert(MacroRef.Target.kind() == Symbol::Macro);
     // FIXME: Handle macro locations.
     return CB(MacroRef,
-              findIncludeHeaders(MacroRef.Target.macro().Definition, SM, PI));
+              findHeaders(MacroRef.Target.macro().Definition, SM, PI));
   }
 }
 
-llvm::SmallVector<Header> findIncludeHeaders(const SymbolLocation &SLoc,
-                                             const SourceManager &SM,
-                                             const PragmaIncludes &PI) {
-  llvm::SmallVector<Header> Results;
-  if (auto *Loc = std::get_if<SourceLocation>(&SLoc)) {
-    // FIXME: Handle non self-contained files.
-    FileID FID = SM.getFileID(*Loc);
-    const auto *FE = SM.getFileEntryForID(FID);
-    if (!FE)
-      return {};
-
-    // We treat the spelling header in the IWYU pragma as the final public
-    // header.
-    // FIXME: look for exporters if the public header is exported by another
-    // header.
-    llvm::StringRef VerbatimSpelling = PI.getPublic(FE);
-    if (!VerbatimSpelling.empty())
-      return {Header(VerbatimSpelling)};
-
-    Results = {Header(FE)};
-    // FIXME: compute transitive exporter headers.
-    for (const auto *Export : PI.getExporters(FE, SM.getFileManager()))
-      Results.push_back(Export);
-    return Results;
-  }
-  if (auto *Sym = std::get_if<tooling::stdlib::Symbol>(&SLoc)) {
-    for (const auto &H : Sym->headers())
-      Results.push_back(H);
-    return Results;
-  }
-  llvm_unreachable("unhandled SymbolLocation kind!");
-}
-
 } // namespace clang::include_cleaner

clang-tools-extra/include-cleaner/lib/AnalysisInternal.h

 /// - the RefType describes the relation between the SourceLocation and the
 ///   NamedDecl.
 ///
 /// walkAST is typically called once per top-level declaration in the file
 /// being analyzed, in order to find all references within it.
 void walkAST(Decl &Root,
              llvm::function_ref<void(SourceLocation, NamedDecl &, RefType)>);
 
-/// A location where a symbol can be provided.
+/// A place where a symbol can be provided.
 /// It is either a physical file of the TU (SourceLocation) or a logical
 /// location in the standard library (stdlib::Symbol).
-// FIXME: use a real Class!
-using SymbolLocation = std::variant<SourceLocation, tooling::stdlib::Symbol>;
+struct SymbolLocation {
+  enum Kind {
+    /// A position within a source file (or macro expansion) parsed by clang.
+    Physical,
+    /// A recognized standard library symbol, like std::string.
+    Standard,
+  };
+
+  SymbolLocation(SourceLocation S) : Storage(S) {}
+  SymbolLocation(tooling::stdlib::Symbol S) : Storage(S) {}
+
+  Kind kind() const { return static_cast<Kind>(Storage.index()); }
+  bool operator==(const SymbolLocation &RHS) const {
+    return Storage == RHS.Storage;
+  }
+
+  SourceLocation physical() const { return std::get<Physical>(Storage); }
+  tooling::stdlib::Symbol standard() const {
+    return std::get<Standard>(Storage);
+  }
+
+private:
+  // Order must match Kind enum!
+  std::variant<SourceLocation, tooling::stdlib::Symbol> Storage;
+};
+llvm::raw_ostream &operator<<(llvm::raw_ostream &, const Header &);
 
 /// Finds the headers that provide the symbol location.
 // FIXME: expose signals
-llvm::SmallVector<Header> findIncludeHeaders(const SymbolLocation &Loc,
-                                             const SourceManager &SM,
-                                             const PragmaIncludes &PI);
+llvm::SmallVector<Header> findHeaders(const SymbolLocation &Loc,
 
 /// Write an HTML summary of the analysis to the given stream.
 /// FIXME: Once analysis has a public API, this should be public too.
 void writeHTMLReport(FileID File, llvm::ArrayRef<Decl *> Roots, ASTContext &Ctx,
                      llvm::raw_ostream &OS);
 
 } // namespace include_cleaner
 } // namespace clang
 
 #endif

clang-tools-extra/include-cleaner/lib/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS Support)
 
 add_clang_library(clangIncludeCleaner
   Analysis.cpp
+  FindHeaders.cpp
   HTMLReport.cpp
+  LocateSymbol.cpp
   Record.cpp
   Types.cpp
   WalkAST.cpp
 
   LINK_LIBS
   clangAST
   clangBasic
   clangLex
   clangToolingInclusionsStdlib
   )
 

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

+//===--- FindHeaders.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/Record.h"
+#include "clang/Basic/SourceManager.h"
+
+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 non self-contained files.
+    FileID FID = SM.getFileID(Loc.physical());
+    const auto *FE = SM.getFileEntryForID(FID);
+    if (!FE)
+      return {};
+
+    // We treat the spelling header in the IWYU pragma as the final public
+    // header.
+    // FIXME: look for exporters if the public header is exported by another
+    // header.
+    llvm::StringRef VerbatimSpelling = PI.getPublic(FE);
+    if (!VerbatimSpelling.empty())
+      return {Header(VerbatimSpelling)};
+
+    Results = {Header(FE)};
+    // FIXME: compute transitive exporter headers.
+    for (const auto *Export : PI.getExporters(FE, SM.getFileManager()))
+      Results.push_back(Export);
+    return Results;
+  }
+  case SymbolLocation::Standard: {
+    for (const auto &H : Loc.standard().headers())
+      Results.push_back(H);
+    return Results;
+  }
+  }
+  llvm_unreachable("unhandled SymbolLocation kind!");
+}
+
+} // namespace clang::include_cleaner
 No newline at end of file

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

+//===--- LocateSymbol.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 "llvm/ADT/StringExtras.h"
+#include "llvm/Support/raw_ostream.h"
+
+namespace clang::include_cleaner {
+
+llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const SymbolLocation &S) {
+  switch (S.kind()) {
+  case SymbolLocation::Physical:
+    // We can't decode the Location without SourceManager. Its raw
+    // representation isn't completely useless (and distinguishes
+    // SymbolReference from Symbol).
+    return OS << "@0x"
+              << llvm::utohexstr(
+                     S.physical().getRawEncoding(), /*LowerCase=*/false,
+                     /*Width=*/CHAR_BIT * sizeof(SourceLocation::UIntTy));
+  case SymbolLocation::Standard:
+    return OS << S.standard().scope() << S.standard().name();
+  }
+  llvm_unreachable("Unhandled Symbol kind");
+}
+
+} // namespace clang::include_cleaner
 No newline at end of file

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

 //===--- AnalysisTest.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 "clang-include-cleaner/Analysis.h"
-#include "AnalysisInternal.h"
 #include "clang-include-cleaner/Record.h"
 #include "clang-include-cleaner/Types.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Testing/TestAST.h"
walkUsed(/*ASTRoots=*/{}, /*MacroRefs=*/
              OffsetToProviders.try_emplace(Offset, Providers.vec());
            });
 
   EXPECT_THAT(
       OffsetToProviders,
       UnorderedElementsAre(Pair(Main.point(), UnorderedElementsAre(HdrFile))));
 }
 
-TEST(FindIncludeHeaders, IWYU) {
-  TestInputs Inputs;
-  PragmaIncludes PI;
-  Inputs.MakeAction = [&PI] {
-    struct Hook : public PreprocessOnlyAction {
-    public:
-      Hook(PragmaIncludes *Out) : Out(Out) {}
-      bool BeginSourceFileAction(clang::CompilerInstance &CI) override {
-        Out->record(CI);
-        return true;
-      }
-
-      PragmaIncludes *Out;
-    };
-    return std::make_unique<Hook>(&PI);
-  };
-
-  Inputs.Code = R"cpp(
-    #include "header1.h"
-    #include "header2.h"
-  )cpp";
-  Inputs.ExtraFiles["header1.h"] = R"cpp(
-    // IWYU pragma: private, include "path/public.h"
-  )cpp";
-  Inputs.ExtraFiles["header2.h"] = R"cpp(
-    #include "detail1.h" // IWYU pragma: export
-
-    // IWYU pragma: begin_exports
-    #include "detail2.h"
-    // IWYU pragma: end_exports
-
-    #include "normal.h"
-  )cpp";
-  Inputs.ExtraFiles["normal.h"] = Inputs.ExtraFiles["detail1.h"] =
-      Inputs.ExtraFiles["detail2.h"] = "";
-  TestAST AST(Inputs);
-  const auto &SM = AST.sourceManager();
-  auto &FM = SM.getFileManager();
-  // Returns the source location for the start of the file.
-  auto SourceLocFromFile = [&](llvm::StringRef FileName) {
-    return SM.translateFileLineCol(FM.getFile(FileName).get(),
-                                   /*Line=*/1, /*Col=*/1);
-  };
-
-  EXPECT_THAT(findIncludeHeaders(SourceLocFromFile("header1.h"), SM, PI),
-              UnorderedElementsAre(Header("\"path/public.h\"")));
-
-  EXPECT_THAT(findIncludeHeaders(SourceLocFromFile("detail1.h"), SM, PI),
-              UnorderedElementsAre(Header(FM.getFile("header2.h").get()),
-                                   Header(FM.getFile("detail1.h").get())));
-  EXPECT_THAT(findIncludeHeaders(SourceLocFromFile("detail2.h"), SM, PI),
-              UnorderedElementsAre(Header(FM.getFile("header2.h").get()),
-                                   Header(FM.getFile("detail2.h").get())));
-
-  EXPECT_THAT(findIncludeHeaders(SourceLocFromFile("normal.h"), SM, PI),
-              UnorderedElementsAre(Header(FM.getFile("normal.h").get())));
-}
 
 } // namespace
 } // namespace clang::include_cleaner

clang-tools-extra/include-cleaner/unittests/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   Support
   TestingSupport
   )
 
 add_custom_target(ClangIncludeCleanerUnitTests)
 add_unittest(ClangIncludeCleanerUnitTests ClangIncludeCleanerTests
   AnalysisTest.cpp
+  FindHeadersTest.cpp
   RecordTest.cpp
   WalkASTTest.cpp
 )
 
 target_include_directories(ClangIncludeCleanerTests
   PRIVATE
   ${CMAKE_CURRENT_SOURCE_DIR}/../lib)
 

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/Record.h"
+#include "clang/Frontend/FrontendActions.h"
+#include "clang/Testing/TestAST.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang::include_cleaner {
+namespace {
+using testing::UnorderedElementsAre;
+
+TEST(FindIncludeHeaders, IWYU) {
+  TestInputs Inputs;
+  PragmaIncludes PI;
+  Inputs.MakeAction = [&PI] {
+    struct Hook : public PreprocessOnlyAction {
+    public:
+      Hook(PragmaIncludes *Out) : Out(Out) {}
+      bool BeginSourceFileAction(clang::CompilerInstance &CI) override {
+        Out->record(CI);
+        return true;
+      }
+
+      PragmaIncludes *Out;
+    };
+    return std::make_unique<Hook>(&PI);
+  };
+
+  Inputs.Code = R"cpp(
+    #include "header1.h"
+    #include "header2.h"
+  )cpp";
+  Inputs.ExtraFiles["header1.h"] = R"cpp(
+    // IWYU pragma: private, include "path/public.h"
+  )cpp";
+  Inputs.ExtraFiles["header2.h"] = R"cpp(
+    #include "detail1.h" // IWYU pragma: export
+
+    // IWYU pragma: begin_exports
+    #include "detail2.h"
+    // IWYU pragma: end_exports
+
+    #include "normal.h"
+  )cpp";
+  Inputs.ExtraFiles["normal.h"] = Inputs.ExtraFiles["detail1.h"] =
+      Inputs.ExtraFiles["detail2.h"] = "";
+  TestAST AST(Inputs);
+  const auto &SM = AST.sourceManager();
+  auto &FM = SM.getFileManager();
+  // Returns the source location for the start of the file.
+  auto SourceLocFromFile = [&](llvm::StringRef FileName) {
+    return SM.translateFileLineCol(FM.getFile(FileName).get(),
+                                   /*Line=*/1, /*Col=*/1);
+  };
+
+  EXPECT_THAT(findHeaders(SourceLocFromFile("header1.h"), SM, PI),
+              UnorderedElementsAre(Header("\"path/public.h\"")));
+
+  EXPECT_THAT(findHeaders(SourceLocFromFile("detail1.h"), SM, PI),
+              UnorderedElementsAre(Header(FM.getFile("header2.h").get()),
+                                   Header(FM.getFile("detail1.h").get())));
+  EXPECT_THAT(findHeaders(SourceLocFromFile("detail2.h"), SM, PI),
+              UnorderedElementsAre(Header(FM.getFile("header2.h").get()),
+                                   Header(FM.getFile("detail2.h").get())));
+
+  EXPECT_THAT(findHeaders(SourceLocFromFile("normal.h"), SM, PI),
+              UnorderedElementsAre(Header(FM.getFile("normal.h").get())));
+}
+
+} // namespace
+} // namespace clang::include_cleaner
 No newline at end of file