ioeric added a comment.
Thanks for the reviews!
================
Comment at: clangd/ClangdUnit.cpp:155
+ auto S = FS->status(Path);
+ if (S)
+ cacheStatus(*S);
----------------
sammccall wrote:
> why are you only caching success?
I had a comment in `openFileForRead`:
```
// Only cache stats for files that exist because, unlike building preamble,
// we only care about existing files when reusing preamble.
```
Another reason is that we are using the file name in the Status as the cache
key.
================
Comment at: clangd/ClangdUnit.cpp:339
+ llvm::ErrorOr<vfs::Status> status(const Twine &Path) override {
+ auto I = StatCache.find(Path.str());
+ return (I != StatCache.end()) ? I->getValue() : FS->status(Path);
----------------
ilya-biryukov wrote:
> We store absolute paths, but Path can be relative here.
This is because preamble stores absolute paths. `Path` should be an path from
preamble. Added documentation.
================
Comment at: clangd/CodeComplete.cpp:1028
+ IntrusiveRefCntPtr<vfs::FileSystem> VFS = Input.VFS;
+ if (Input.PreambleStatCache)
----------------
ilya-biryukov wrote:
> It feels like we could apply this caching one level higher, on the
> TUScheduler level when creating the filesystem. It makes sense not only for
> code completion, but also for all other features, including rebuilds of ASTs
> that were washed out of the cache.
> WDYT?
It sounds like a reasonable thing to do. I took a quick look, and it seemed
that this would probably require some refactoring/redesign on TUScheduler - it
doesn't know about the VFS?
I think it shouldn't be hard to do this case by case. For example, code
completion and signature help will be covered in this patch. And it should be
pretty easy to make AST rebuild use the cache as well?
================
Comment at: clangd/CodeComplete.h:217
+CodeCompleteResult codeComplete(PathRef FileName, Position Pos,
+ const InputsAndPreamble &Input,
IntrusiveRefCntPtr<vfs::FileSystem> VFS,
----------------
sammccall wrote:
> ilya-biryukov wrote:
> > We should not be depending `TUScheduler.h`, I suggest to pass
> > `PreambleData`instead of `PrecompiledPreamble` instead.
> >
> > Depending on `TUScheduler.h` our layering in clangd: low-level
> > implementation of features should not depend on higher-level building
> > blocks like TUScheduler.
> +1. We're maybe not doing the best job of header encapsulation here, but
> InputsAndPreamble is an implementation detail of ClangdServer.
I thought `InputsAndPreamble` was a pretty natural fit here, but I guess I
hadn't understand the abstractions well enough. Thanks for the explanation!
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52419
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
