[PATCH] D54269: Introduce shard storage to auto-index.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

just some nits. Main thing is the LoggingStorage - again I think this may have been a misunderstanding.



================
Comment at: clangd/index/Background.cpp:76
+    else
+      elog("Error while reading shard {0}: {1}", ShardIdentifier,
+           I.takeError());
----------------
ADD_FAILURE() I think


================
Comment at: clangd/index/Background.cpp:94
+  // directory for all shard files.
+  DiskBackedIndexStorage(llvm::StringRef Directory) {
+    llvm::SmallString<128> CDBDirectory(Directory);
----------------
nit: move constructor to the top?


================
Comment at: clangd/index/Background.cpp:98
+    DiskShardRoot = CDBDirectory.str();
+    if (!llvm::sys::fs::exists(DiskShardRoot)) {
+      std::error_code OK;
----------------
create_directory returns success if the directory exists, so no need for this check


================
Comment at: clangd/index/Background.cpp:108
+
+class LoggingIndexStorage : public BackgroundIndexStorage {
+public:
----------------
Hmm, I'm not really sure what this is for - does someone constructing BackgroundIndex ever not want to define storage *and* not want to control logging?


================
Comment at: clangd/index/Background.cpp:344
+      if (auto Error = IndexStorage->storeShard(Path, Shard))
+        elog("Failed to store shard for {0}: {1}", Path, std::move(Error));
+    }
----------------
this doesn't give enough context - the logger is global to clangd.

maybe "Failed to write background-index shard for file {0}: {1}"


================
Comment at: clangd/index/Background.h:39
+
+  virtual llvm::Expected<IndexFileIn>
+  loadShard(llvm::StringRef ShardIdentifier) const = 0;
----------------
kadircet wrote:
> sammccall wrote:
> > sammccall wrote:
> > > docs
> > Hmm, we're going to attempt to load the shard corresponding to every file that we have in the CDB, even if the index isn't built yet. So "file doesn't exist" is an expected case (where we don't want to log etc), vs e.g. "file was corrupted" is unexpected and should definitely be logged. How do we distinguish these with this API?
> > 
> > One defensible option would be to have implementations handle errors: return Optional<IndexFileIn>, and have the disk version log errors appropriately and return None.
> > 
> > Another would be Expected<Optional<IndexFileIn>> or so, which is a little messy.
> used the former proposition, but with std::unique_ptr instead of llvm::Optional since IndexFileIn is move-only.
This is fine, though Optional works fine with move-only types.

(your comment still says None)


================
Comment at: clangd/index/Background.h:48
+  static BackgroundIndexStorage *
+  createDiskStorage(llvm::StringRef CDBDirectory);
+};
----------------
we may also want to provide the standard factory - can go in a later patch though.


================
Comment at: unittests/clangd/BackgroundIndexTests.cpp:127
+      [&Storage, &CacheHits](llvm::StringRef) {
+        static MemoryShardStorage MSS(Storage, CacheHits);
+        return &MSS;
----------------
nit: seems a little less unusual to avoid the static:
```
MemoryShardStorage Storage;
auto MemoryStorageFactory = [&](llvm::StringRef){ return &Storage; }
...
EXPECT_THAT(Storage.contains("root/A.h"));
```


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54269