[PATCH] D64147: [clangd] Make HadErrors part of background index's internal state

[Sam McCall via Phabricator via cfe-commits]

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