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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:262
+         ID != SM.getMainFileID() && FE &&
+         !isSelfContainedHeader(PP, ID, FE);) {
+      ID = SM.getFileID(SM.getIncludeLoc(ID));
----------------
kbobyrev wrote:
> sammccall wrote:
> > 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.
> I am slightly confused: we don't really have the `IncludeStructure` here and it is logically detached from the `IncludeStructure` processing. We'd still have to unroll the chain of FIDs in here, so the only difference would be querying `IncludeStructure` data for the cached `isSelfContainedHeader` value, is that right? Why is obtaining that info at preamble build time better?
the call-site has access to ParsedAST, hence the IncludeStructure. I believe the main reasoning behind Sam's suggestion is performing these lookups once while building the preamble and sharing it afterwards (we can even do the IO there).


================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:56
+// Is Line an #if or #ifdef directive?
+static bool isIf(llvm::StringRef Line) {
+  Line = Line.ltrim();
----------------
no need for static here (and other places below)


================
Comment at: clang-tools-extra/clangd/SourceCode.h:332
+bool isSelfContainedHeader(const Preprocessor &PP, FileID FID,
+                           const FileEntry *FE, bool ExpensiveCheck = false);
+
----------------
i'd suggest `AllowIO` rather than `ExpensiveCheck`


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:283
   // Is Line an #if or #ifdef directive?
   static bool isIf(llvm::StringRef Line) {
     Line = Line.ltrim();
----------------
can you also delete this and the other helpers?


================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:346
+  // headers to header-guarded ones.
+  for (const auto &Key : ReferencedFileNames.keys()) {
+    llvm::outs() << "Key: " << Key << '\n';
----------------
looks like a debug artifact


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114370