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