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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks, this mostly LG now.
I'd consider splitting out the new infra change (IncludeStructure) from the feature change (include-cleaner treatment).
In case the latter causes some problems...



================
Comment at: clang-tools-extra/clangd/Headers.cpp:92
         InBuiltinFile = false;
+      // At the file exit time HeaderSearchInfo is valid and can be used to
+      // determine whether the file was a self-contained header or not.
----------------
nit: at the file exit time -> at file exit time


================
Comment at: clang-tools-extra/clangd/Headers.h:65
   SrcMgr::CharacteristicKind FileKind = SrcMgr::C_User;
+  bool SelfContained = true;
   llvm::Optional<unsigned> HeaderID;
----------------
Why redundantly track this on Inclusion? It's already available via IncludeStructure.SelfContained.contains(*HeaderID)


================
Comment at: clang-tools-extra/clangd/Headers.h:128
   // populates the structure.
-  std::unique_ptr<PPCallbacks> collect(const SourceManager &SM);
+  std::unique_ptr<PPCallbacks> collect(const SourceManager &SM,
+                                       HeaderSearch &HeaderInfo);
----------------
Maybe it's neater just to pass the CompilerInstance& - there's no other practical way to use this.


================
Comment at: clang-tools-extra/clangd/Headers.h:158
+  // Contains HeaderIDs of all self-contained entries in the IncludeStructure.
+  llvm::DenseSet<HeaderID> SelfContained;
+
----------------
I think i'd prefer to see this private with an `isSelfContained` accessor, because the representation isn't the only sensible one.

Case in point: almost all headers in practice are self-contained, so in fact storing the set of non-self-contained headers seems preferable.


================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:53
 
+namespace {
+
----------------
these helpers are 1000 lines away from the only place they're used, please move them closer


================
Comment at: clang-tools-extra/clangd/SourceCode.h:330
+/// guard, does not rely on preprocessor state). This will read a portion of the
+/// file which might be discouraged in performance-sensitive context.
+bool isSelfContainedHeader(const FileEntry *FE, FileID ID,
----------------
This still doesn't really describe the situation:
 - if you're not getting the header from a preamble, it won't have to read anything as it'll be in-memory already
 - if you are, then the fact that the content may have changed is at least as big a problem
 - the string scanning itself is not ideal to do redundantly from a performance POV

I'd rather say:
"This scans source code, and should not be called when using a preamble.
Prefer to access the cache in IncludeStructure::isSelfContained if you can."


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114370