[PATCH] D108194: [clangd] IncludeCleaner: Mark used headers

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tools-extra/clangd/Headers.h:65
   SrcMgr::CharacteristicKind FileKind = SrcMgr::C_User;
+  unsigned ID = std::numeric_limits<unsigned>::max(); // Corresponding HeaderID.
 };
----------------
I don't think we're under any size pressure here - `Optional<unsigned>`?


================
Comment at: clang-tools-extra/clangd/Headers.h:65
   SrcMgr::CharacteristicKind FileKind = SrcMgr::C_User;
+  unsigned ID = std::numeric_limits<unsigned>::max(); // Corresponding HeaderID.
 };
----------------
sammccall wrote:
> I don't think we're under any size pressure here - `Optional<unsigned>`?
call the member HeaderID, rather than have this as a comment only?


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:161
+          const llvm::DenseSet<IncludeStructure::HeaderID> &ReferencedFiles,
+          const SourceManager &SM) {
+  std::vector<const Inclusion *> Unused;
----------------
SM is unused


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:165
+    // FIXME: Skip includes that are not self-contained.
+    if (MFI.Resolved.empty()) {
+      elog("{0} is unresolved", MFI.Written);
----------------
this can just be a check whether MFI.ID is valid or not


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:171
+    assert(IncludeID != IncludeStructure::HeaderID::Invalid);
+    bool Used = ReferencedFiles.find(IncludeID) != ReferencedFiles.end();
+    if (!Used) {
----------------
ReferencedFiles.contains(IncludeID) and inline into the if?


================
Comment at: clang-tools-extra/clangd/ParsedAST.h:162
 
+std::vector<const Inclusion *> computeUnusedIncludes(ParsedAST &AST);
+
----------------
I think this function belongs in IncludeCleaner.h.

(There's a circularity problem between ParsedAST and IncludeCleaner, but putting the function here doesn't fix it)


================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:165
+  for (const auto &Include : UnusedIncludes) {
+    // Strip enclosing "".
+    UnusedHeaders.push_back(
----------------
Don't bother doing this stripping just for the test IMO, it obscures the assertion (more than escaping quotes would)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108194