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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Refactoring generally looks good.
You're replacing a lot of documented code with new undocumented code, can we add some high-level comments?



================
Comment at: clang-tools-extra/clangd/index/Background.cpp:130
+
+bool hasChanged(llvm::vfs::FileSystem *FS, const ShardInfo &SI) {
+  auto Buf = FS->getBufferForFile(SI.AbsolutePath);
----------------
nit: unclear what's changed with respect to what.

I'd suggest shardIsStale() and swap the param order?


================
Comment at: clang-tools-extra/clangd/index/Background.cpp:133
+  if (!Buf) {
+    elog("Couldn't get buffer for file: {0}: {1}", SI.AbsolutePath,
+         Buf.getError().message());
----------------
While here: this message lacks context and is unnecessarily jargony: "Background-indexing: couldn't read {0} to validate stored index"?


================
Comment at: clang-tools-extra/clangd/index/Background.cpp:455
+// TUs that had out-of-date/no shards.
+std::vector<std::pair<tooling::CompileCommand, BackgroundIndexStorage *>>
+BackgroundIndex::loadProject(std::vector<std::string> MainFiles) {
----------------
This function now has almost no comments, can you explain the major steps?


================
Comment at: clang-tools-extra/clangd/index/Background.cpp:481
       SV.HadErrors = SI.HadErrors;
+      Rebuilder.loadedShard();
 
----------------
move this outside the loop and use a counter? it's an extra lock/unlock for no reason


================
Comment at: clang-tools-extra/clangd/index/Background.cpp:490
+  auto FS = FSProvider.getFileSystem();
+  llvm::DenseSet<PathRef> TUsToIndex;
+  for (auto &SI : Result.Shards) {
----------------
nit: again, this patch is adding Path/PathRef aliases to parts of the code that don't use them


================
Comment at: clang-tools-extra/clangd/index/Background.h:177
   BackgroundIndexStorage::Factory IndexStorageFactory;
-  struct Source {
-    std::string Path;
-    bool NeedsReIndexing;
-    Source(llvm::StringRef Path, bool NeedsReIndexing)
-        : Path(Path), NeedsReIndexing(NeedsReIndexing) {}
-  };
-  // Loads the shards for a single TU and all of its dependencies. Returns the
-  // list of sources and whether they need to be re-indexed.
-  std::vector<Source> loadShard(const tooling::CompileCommand &Cmd,
-                                BackgroundIndexStorage *IndexStorage,
-                                llvm::StringSet<> &LoadedShards);
-  // Tries to load shards for the ChangedFiles.
+  // Tries to load shards for the MainFiles.
   std::vector<std::pair<tooling::CompileCommand, BackgroundIndexStorage *>>
----------------
and their dependencies


================
Comment at: clang-tools-extra/clangd/index/BackgroundIndexLoader.cpp:77
+  for (const auto &It : *SI.Shard->Sources) {
+    auto AbsPath = uriToAbsolutePath(It.getKey(), ShardIdentifier);
+    if (!AbsPath || *AbsPath != ShardIdentifier) {
----------------
you can't call this an (opaque?) ShardIdentifier and then use it as a path - maybe SourceFile?


================
Comment at: clang-tools-extra/clangd/index/BackgroundIndexLoader.h:1
+//===--- Background.h - Build an index in a background thread ----*- C++-*-===//
+//
----------------
header is out of date. The new file/structs/functions need documentation


================
Comment at: clang-tools-extra/clangd/index/BackgroundIndexLoader.h:25
+
+struct ShardInfo {
+  Path AbsolutePath;
----------------
LoadedShard?


================
Comment at: clang-tools-extra/clangd/index/BackgroundIndexLoader.h:30
+  bool HadErrors = false;
+  std::unique_ptr<IndexFileIn> Shard;
+};
----------------
IndexFileIn is just a struct - optional<>? (And need a comment explaining when it can be null)


================
Comment at: clang-tools-extra/clangd/index/BackgroundIndexLoader.h:36
+  /// Keeps a mapping from a given file to TUs that are depending on it.
+  /// Strings are owned by the \p Shards.
+  llvm::StringMap<std::vector<Path>> FileToTUs;
----------------
no, they're not :-)

we talked about making the values refs to ShardInfo.AbsolutePath.


================
Comment at: clang-tools-extra/clangd/index/BackgroundIndexLoader.h:37
+  /// Strings are owned by the \p Shards.
+  llvm::StringMap<std::vector<Path>> FileToTUs;
+};
----------------
why are these outside the ShardInfo?


================
Comment at: clang-tools-extra/clangd/index/BackgroundIndexLoader.h:42
+/// any shards that are first seen for this TU are stale.
+ShardLoadResult load(llvm::ArrayRef<Path> MainFiles,
+                     BackgroundIndexStorage::Factory &IndexStorageFactory,
----------------
clangd::load seems like an insufficiently precise name for this function

loadIndexShards?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64712