[PATCH] D64712: [clangd] Refactor background-index shard loading

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/index/Background.cpp:133
+  if (!Buf) {
+    elog("Background-index: Couldn't read {0 to validate stored index: {1}",
+         LS.AbsolutePath, Buf.getError().message());
----------------
`{0` is missing its `}`


================
Comment at: clang-tools-extra/clangd/index/Background.cpp:497
+  llvm::DenseSet<PathRef> TUsToIndex;
+  // Check for staleness of loaded shards.
+  for (auto &LS : Result) {
----------------
// We'll accept data from stale shards, but ensure the files get reindexed soon.


================
Comment at: clang-tools-extra/clangd/index/BackgroundIndexLoader.cpp:93
+
+void BackgroundIndexLoader::load(PathRef MainFile,
+                                 BackgroundIndexStorage *Storage) {
----------------
This handles only one main file, I can't see where you skip loading shards for headers that were loaded for a previous file


================
Comment at: clang-tools-extra/clangd/index/BackgroundIndexLoader.cpp:104
+  while (!ToVisit.empty()) {
+    PathRef ShardIdentifier = ToVisit.front();
+    ToVisit.pop();
----------------
ShardIdentifier still here


================
Comment at: clang-tools-extra/clangd/index/BackgroundIndexLoader.cpp:126
+    assert(TUsIt != FileToTUs.end() && "No TU registered for the shard");
+    Result.back().DependentTUs = std::move(TUsIt->second);
+  }
----------------
I don't think this works, the storage for these StringRefs is the LoadedShards you just moved from.
(In practice, maybe you get away with it if the strings are longer than the SSO?)




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64712