sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Logic looks great, just naming/comment nits
================
Comment at: clang-tools-extra/clangd/index/Background.cpp:276
+ const auto DigestIt = IndexedFilesSnapshot.find(AbsPath);
// File has different contents.
+ if (DigestIt == IndexedFilesSnapshot.end() ||
----------------
, or we indexed successfully this time.
================
Comment at: clang-tools-extra/clangd/index/Background.cpp:444
SymbolCollector::Options IndexOpts;
- IndexOpts.FileFilter = createFileFilter(DigestsSnapshot);
+ // Creates a filter to not collect index results from files with unchanged
+ // digests.
----------------
I'm happy with this inline or out-of-line, but if it's inline here it shouldn't
have a function-style comment with \p :-)
================
Comment at: clang-tools-extra/clangd/index/Background.h:98
+ /// Represents the state of a single file when indexing was performed.
+ struct IndexedFile {
+ FileDigest Digest{0};
----------------
I wonder whether a more "flavorful" name like "ShardVersion" might help explain
the purpose here?
================
Comment at: clang-tools-extra/clangd/index/Background.h:125
FileSymbols IndexedSymbols;
- llvm::StringMap<FileDigest> IndexedFileDigests; // Key is absolute file path.
+ // We do not store the whole IncludeGraph since we only care about Digest and
+ // Errorness of each source file. We might start storing IncludeGraph
directly
----------------
It's not obvious to me why we're apologising for this not being the include
graph. The data happens to come from there (kind of) but I wouldn't really
mention it.
================
Comment at: clang-tools-extra/clangd/index/Background.h:128
+ // once there are use cases for additional information in include graph.
+ llvm::StringMap<IndexedFile> IndexedFiles; // Key is absolute file path.
std::mutex DigestsMu;
----------------
Again, I think "Versions" would make it more obvious what this is for.
================
Comment at: clang-tools-extra/clangd/index/Background.h:129
+ llvm::StringMap<IndexedFile> IndexedFiles; // Key is absolute file path.
std::mutex DigestsMu;
----------------
rename mutex to match guarded variable
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64147/new/
https://reviews.llvm.org/D64147
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
