[PATCH] D137320: [include-cleaner] Initial version for the "Location=>Header" step

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h:54
   Header(tooling::stdlib::Header H) : Storage(H) {}
+  Header(llvm::StringRef VerbatimSpelling) : Storage(VerbatimSpelling.str()) {}
 
----------------
can you add comments here describing what this case is used for


================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h:63
+// FIXME: use a real Class!
+using SymbolLocation = std::variant<SourceLocation, tooling::stdlib::Symbol>;
+
----------------
let's move this to `AnalysisInternal.h`, as it isn't used in any of the public apis.


================
Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:45
+  llvm::SmallVector<Header> Results;
+  if (SLoc.index() == 0) {
+    // FIXME: Handle non self-contained files.
----------------
nit: `if (auto *Loc = std::get_if<SourceLocation>(&SLoc))`


================
Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:55
+    if (!VerbatimSpelling.empty())
+      return {{VerbatimSpelling}};
+
----------------
what about exporters here?


================
Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:58
+    Results = {{FE}};
+    // FIXME: compute a closure of exporter headers.
+    for (const auto *Export : PI.getExporters(FE, SM.getFileManager()))
----------------
not sure i follow the fixme here, are you suggesting we should also compute transitive exporters?


================
Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:63
+  }
+  if (SLoc.index() == 1) {
+    for (const auto &H : std::get<tooling::stdlib::Symbol>(SLoc).headers())
----------------
nit: `if (auto *Sym = std::get<tooling::stdlib::Symbol>(&SLoc))`


================
Comment at: clang-tools-extra/include-cleaner/lib/AnalysisInternal.h:49
+// FIXME: expose signals
+llvm::SmallVector<Header> findIncludeHeaders(const SymbolLocation &Loc,
+                                             const SourceManager &SM,
----------------
this is fine for the initial version, but we'll likely lose a lot of performance here going forward. we can address it later on though, as this is an internal api and those performance concerns won't be a problem until this is used more widely. 

we're likely going to call this function multiple times for each symbol, and also despite having different sourcelocations, there'll probably be lots of declarations that're originating from the same FileID.
Hence we'll want to accumulate outputs on a single container, rather than returning a new container at each call, and also have some sort of file-id based cache.


================
Comment at: clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp:116
+
+  EXPECT_THAT(walk(AST),
+              UnorderedElementsAre(
----------------
can we use `findIncludeHeaders` directly?

that way we don't need to set up a referencing file, e.g. you can directly create a SourceLocation inside `private.h` (in this case even the decl is not important). 


(same for the export test)


================
Comment at: clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp:135
+  )cpp";
+  Inputs.ExtraFiles["private.h"] = R"cpp(class Private {};)cpp";
+  TestAST AST(Inputs);
----------------
calling this private is somewhat confusing, maybe call it `Detail`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137320/new/

https://reviews.llvm.org/D137320