sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Thanks! Looks good, rest of the nits are obvious or up to you.(
================
Comment at: clangd/index/Background.cpp:37
-BackgroundIndex::BackgroundIndex(Context BackgroundContext,
- StringRef ResourceDir,
- const FileSystemProvider &FSProvider,
- ArrayRef<std::string> URISchemes,
- size_t ThreadPoolSize)
+namespace {
+
----------------
nit: I think the moves of the helper functions
(digest/digestFile/getAbsoluteFilePath) can be reverted now
================
Comment at: clangd/index/Background.h:45
+
+ using Factory =
+ llvm::unique_function<BackgroundIndexStorage *(llvm::StringRef)>;
----------------
Some docs? Maybe add a comment like
```
// The factory provides storage for each CDB.
// It keeps ownership of the storage instances, and should manage caching
itself.
// Factory must be threadsafe and never returns nullptr.
```
================
Comment at: clangd/index/Background.h:50
+ // CDBDirectory + ".clangd-index/" as the folder to save shards.
+ static Factory getDiskBackedStorageFactory();
+};
----------------
nit: `create` rather than `get` would be clearer that this returns an
independent object each time.
================
Comment at: clangd/index/Background.h:103
+ // index storage
+ BackgroundIndexStorage::Factory IndexStorageFactory;
----------------
(nit: this comment seems redundant with the type/name)
================
Comment at: clangd/index/BackgroundIndexStorage.cpp:30
+ llvm::SmallString<128> ShardRootSS(ShardRoot);
+ llvm::sys::path::append(ShardRootSS, llvm::sys::path::filename(FilePath) +
+ llvm::toHex(digest(FilePath)) +
----------------
nit: can we put a separator like between the path and digest?
I'd suggest `.` so the file extension is most obviously preserved.
e.g. `Foo.cpp.12345.idx` vs `Foo.cpp_123456.idx` vs `Foo.cpp123456.idx'
================
Comment at: clangd/index/BackgroundIndexStorage.cpp:51
+ if (EC != OK) {
+ elog("Failed to create directory {0} for disk backed index storage: {1}",
+ DiskShardRoot, EC.message());
----------------
nit: just "for index storage"?
it's clearly disk backed if it's going into a directory!
(and the extra qualification may confuse users)
================
Comment at: clangd/index/BackgroundIndexStorage.cpp:79
+ OS << Shard;
+ return llvm::Error::success();
+ }
----------------
You're ignoring write errors here (and in fact errors may not have occurred yet
due to buffering).
```
OS.close();
return OS.error();
```
================
Comment at: unittests/clangd/BackgroundIndexTests.cpp:100
+TEST(BackgroundIndexTest, ShardStorageWriteTest) {
+ class MemoryShardStorage : public BackgroundIndexStorage {
+ mutable std::mutex StorageMu;
----------------
Consider pulling this class to the top level, and use it any time you don't
care about the storage (to avoid the need for DummyStorage)
================
Comment at: unittests/clangd/BackgroundIndexTests.cpp:111
+ std::lock_guard<std::mutex> Lock(StorageMu);
+ std::string &str = Storage[ShardIdentifier];
+ llvm::raw_string_ostream OS(str);
----------------
I think this can just be
`Storage[ShardIdentifier] = llvm::to_string(Shard)`
(need to include ScopedPrinters.h)
================
Comment at: unittests/clangd/BackgroundIndexTests.cpp:121
+ if (Storage.find(ShardIdentifier) == Storage.end()) {
+ ADD_FAILURE() << "Shard " << ShardIdentifier << ": not found.";
+ return nullptr;
----------------
this one shouldn't be a failure I think - just return nullptr? (If the test
cares about it, the test should depend on it more explicitly).
The corrupt index file is a more obvious "should never happen" case.
================
Comment at: unittests/clangd/BackgroundIndexTests.cpp:130
+ }
+ CacheHits++;
+ return llvm::make_unique<IndexFileIn>(std::move(*IndexFile));
----------------
You never actually assert on this value.
(I'm not sure you need to, but if not just remove it)
================
Comment at: unittests/clangd/BackgroundIndexTests.cpp:146
+ MemoryShardStorage MSS(Storage, CacheHits);
+ BackgroundIndexStorage::Factory MemoryStorageCreator = [&](llvm::StringRef) {
+ return &MSS;
----------------
seems clearer just to inline this in the constructor(, but up to you
================
Comment at: unittests/clangd/BackgroundIndexTests.cpp:162
+ EXPECT_EQ(Storage.size(), 2U);
+ EXPECT_NE(Storage.find(testPath("root/A.h")), Storage.end());
+ EXPECT_NE(Storage.find(testPath("root/A.cc")), Storage.end());
----------------
(even if the StringMap was encapsulated in the MemoryStorage, you can still
write this assertion through the public interface:
ASSERT_NE(Storage.loadShard("root/A.h"), None)
(you could even assert the actual stored symbols, but may not be worth it)
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
