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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
