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

Sam McCall via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Jul 18 04:26:32 PDT 2019

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?)

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list