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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits