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

Sam McCall via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Oct 5 05:47:25 PDT 2021


sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:159
+std::vector<const Inclusion *>
+getUnused(IncludeStructure::HeaderID EntryPoint,
+          const IncludeStructure &Structure,
----------------
EntryPoint is unused


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:166
+    // FIXME: Skip includes that are not self-contained.
+    auto Entry = SM.getFileManager().getFile(MFI.Resolved);
+    if (!Entry) {
----------------
Handle unresolved case somehow? (Or assert if you're sure it can't happen - I think it can for e.g. pp_file_not_found)


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:166
+    // FIXME: Skip includes that are not self-contained.
+    auto Entry = SM.getFileManager().getFile(MFI.Resolved);
+    if (!Entry) {
----------------
sammccall wrote:
> Handle unresolved case somehow? (Or assert if you're sure it can't happen - I think it can for e.g. pp_file_not_found)
doesn't seem like there's any need to go through filenames for this.
Can't we just store the HeaderID in the Inclusion?
(Blech, as an `unsigned` to avoid a circular dependency)


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:168
+    if (!Entry) {
+      elog("Missing FileEntry for {0}", MFI.Resolved);
+      continue;
----------------
elog says that:
a) this might happen
b) logging for the user is the best thing we can do

Can this actually happen? My suspicion is no. In which case maybe it should be an assert?


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:173
+    if (!It) {
+      elog("Missing IncludeStructure::File for {0}", MFI.Resolved);
+      continue;
----------------
I'm fairly (more) certain this one should be an assert


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:193
+    if (!FE) {
+      elog("Missing FE for {0}", SM.getComposedLoc(FID, 0).printToString(SM));
+      continue;
----------------
assert?


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:196
+    }
+    const auto File = Includes.getID(FE);
+    if (!File) {
----------------
this is get, not getOrCreate, so you don't need the mutable reference


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:198
+    if (!File) {
+      elog("Missing FE for {0}", SM.getComposedLoc(FID, 0).printToString(SM));
+      continue;
----------------
I'm not totally sure whether this is safe to assert or not. WDYT?

In any case, please fix the message (FE -> HeaderID, add more context)


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:158
+
+std::vector<Inclusion>
+getUnused(IncludeStructure::HeaderID EntryPoint,
----------------
sammccall wrote:
> Why are we passing around Inclusions by value?
Sorry, should have thought this through more before leaving the comment. There are a couple of questions really:

How should we *store* the information about which inclusions are unused?
 - not at all, generate ReferencedFiles and compute "is this header unused" on the fly when generating diagnostics
 - store ReferencedFiles but call "is this header unused" on the fly
 - store a boolean or something in each Inclusion
 - store a list of the inclusions that are unused
IMO this is mostly a question of what's the lifecycle of the info, and what's the simplest representation - seems like we should prefer stuff higher on the list if we have a choice.

What should the signature of the function be?
There doesn't seem to be any work saved here by processing all the includes as a batch - why not simplify by just making this `bool isUnused(...)` and let the caller/test choose what data structures to use?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108194



More information about the cfe-commits mailing list