[PATCH] D54817: [clangd] Put direct headers into srcs section.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

structure and serialization stuff looks great!
still some questions around the auto-index logic and organization of the extraction code, let's chat offline. May make sense to split.



================
Comment at: clangd/index/Background.cpp:329
+llvm::StringMap<IndexFileIn::SourceFile>
+IncludeStructureToSources(llvm::StringRef MainFile,
+                          const IncludeStructure &Includes,
----------------
nit: lowercase


================
Comment at: clangd/index/Background.cpp:341
+    std::string FileURI = URI::create(Dependency).toString();
+    auto Buf = FS->getBufferForFile(Dependency);
+    if (!Buf)
----------------
It looks like you're re-reading the files here. this won't reuse in-memory copies of the file, and won't respect overridden contents outside the VFS (ugh). The SourceManager would be better I think.


================
Comment at: clangd/index/Background.cpp:351
+    SF.Digest = std::move(Digest);
+    for (const auto &Include : Includes.includeDepth(Dependency)) {
+      if (Include.getValue() != 1) // Skip transitive includes.
----------------
I don't think this is going to work:
 - you're outputting the non-transitive dependencies from the TU, but I think you want the *transitive* ones, which should then be partitioned by file (same as symbols). Otherwise the headers from the shard for `foo.h` aren't going to be right, are they?
 - I don't think the current IncludeStructure includes all the info you want for transitive includes. e.g. for include edges A->B where there is a shorter path from Main->B, we don't record anything.
 - I think we will want the include information for other forms of index too. I think this means the logic should be next to symbolcollector, or in Headers.h, where it can be shared


================
Comment at: clangd/index/Serialization.cpp:400
+      Entry->getValue() = std::move(SF);
+      Entry->getValue().FileURI = Entry->getKey();
+      for(auto &Include : Entry->getValue().DirectIncludes)
----------------
these string reassignments deserve a comment


================
Comment at: clangd/index/Serialization.h:41
   using FileDigest = std::array<uint8_t, 20>;
+  // Contains information about one source file that participates in current
+  // index file.
----------------
Does this belong here, vs in Index, or SymbolCollector, or so?


================
Comment at: clangd/index/Serialization.h:53
+  // Keys are URIs of the source files.
+  llvm::Optional<llvm::StringMap<SourceFile>> Sources;
 };
----------------
I think the StringMap could also use a typedef near SourceFile - that's where we could mention that FileURI and DirectIncludes point back at the keys.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54817