sammccall added inline comments.
================
Comment at: clangd/index/Background.cpp:52
+llvm::SmallString<128>
+getShardPathFromFilePath(llvm::SmallString<128> ShardRoot,
+ llvm::StringRef FilePath) {
----------------
nit: these micro-optimizations with SmallString seem unlikely to be worthwhile
- maybe just take StringRef and return string?
================
Comment at: clangd/index/Background.cpp:109
+
+SmallString<128> getAbsolutePath(const tooling::CompileCommand &Cmd) {
+ SmallString<128> AbsolutePath;
----------------
absoluteFilePath? (otherwise it's not clear what the path is to)
================
Comment at: clangd/index/Background.cpp:237
+ if (IndexStorage)
+ loadShards(IndexStorage.get(), Cmds);
+ else
----------------
this absolutely can't be done synchronously (it's much slower than calling
getAllCompileCommands - this could block clangd for tens of seconds when
opening the first file).
Can we drop shard-loading in BackgroundIndex from this patch?
================
Comment at: clangd/index/Background.cpp:256
Queue.push_back(Bind(
- [this](tooling::CompileCommand Cmd) {
+ [this](tooling::CompileCommand Cmd,
+ std::shared_ptr<BackgroundIndexStorage> IndexStorage) {
----------------
you can capture indexstorage by value
================
Comment at: clangd/index/Background.h:34
+private:
+ static
std::function<std::shared_ptr<BackgroundIndexStorage>(llvm::StringRef)>
+ Factory;
----------------
I don't think we should be using a static mutable factory here.
I think I worded the previous comment about exposing `DiskBackedIndexStorage`
badly, what I meant was something like: `static
unique_ptr<BackgroundIndexStorage>
BackgroundIndexStorage::createDiskStorage(StringRef dir)`
This would be separate from how the storage strategy is configured on the
background index.
================
Comment at: clangd/index/Background.h:34
+private:
+ static
std::function<std::shared_ptr<BackgroundIndexStorage>(llvm::StringRef)>
+ Factory;
----------------
sammccall wrote:
> I don't think we should be using a static mutable factory here.
>
> I think I worded the previous comment about exposing `DiskBackedIndexStorage`
> badly, what I meant was something like: `static
> unique_ptr<BackgroundIndexStorage>
> BackgroundIndexStorage::createDiskStorage(StringRef dir)`
>
> This would be separate from how the storage strategy is configured on the
> background index.
>
do we need `shared_ptr` here?
ISTM either the BackgroundIndex is going to be responsible for caching (in
which case it should receive unique_ptr) or the factory is responsible for
caching (in which case it can own the functions, and the BackgroundIndex can
get a non-owning raw pointer)
================
Comment at: clangd/index/Background.h:38
+public:
+ using FileDigest = decltype(llvm::SHA1::hash({}));
+
----------------
unused?
================
Comment at: clangd/index/Background.h:42
+ // retrieved later on with the same identifier.
+ virtual bool storeShard(llvm::StringRef ShardIdentifier,
+ IndexFileOut Shard) const = 0;
----------------
Prefer llvm::Error over bool, or no return value if you don't think we ever
want to handle it.
================
Comment at: clangd/index/Background.h:45
+
+ // Retrieves the shard if found, also returns hash for the source file that
+ // generated this shard.
----------------
doc stale
================
Comment at: clangd/index/Background.h:48
+ virtual llvm::Expected<IndexFileIn>
+ retrieveShard(llvm::StringRef ShardIdentifier) const = 0;
+
----------------
nit: I think "loadShard" would better emphasize the relationship with
`storeShard`, as load/store is such a common pair. (read/write would similarly
work).
Nothing wrong with "retrieve" per se, I just think the pairing is better.
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
