[PATCH] D114370: [clangd] IncludeCleaner: Attribute symbols from non self-contained headers to their parents

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:252
   }
-  return std::move(Result.Files);
+  // Post-filtering attributes the locations from non self-contained headers to
+  // their parents while the information about respective SourceLocations and
----------------
This comment seems a bit unclear:
 - what the problem is you're solving
 - whether you're describing the behavior you *want* rather than the behavior you're trying to avoid.
 - which part of this is "filtering"
 - what the "later" option is you're contrasting to

Maybe something like:

```
// If a header is not self-contained, we consider its symbols a logical part of the including file.
// Therefore, mark the parents of all used non-self-contained FileIDs as used.
// Perform this on FileIDs rather than HeaderIDs, as each inclusion of a non-self-contained file is distinct.
```


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:262
+         ID != SM.getMainFileID() && FE &&
+         !isSelfContainedHeader(PP, ID, FE);) {
+      ID = SM.getFileID(SM.getIncludeLoc(ID));
----------------
it seems like we'd be better off storing the "is-self-contained" in the IncludeStructure and looking up the HeaderID here rather than asking the preprocessor. That way we rely on info that's better obtained at preamble build time.


================
Comment at: clang-tools-extra/clangd/SourceCode.h:330
+/// preprocessor state).
+bool isSelfContainedHeader(const Preprocessor &PP, FileID FID,
+                           const FileEntry *FE);
----------------
This helper checks e.g. for "don't include me", which is going to read source code of preamble files - we shouldn't do that, it's too slow and racy to do for every file in the preamble.

(It would be nice to handle those cases at some point, if we want to do that we need to do it at preamble build time and record the results in the IncludeStructure)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114370