kadircet added a comment.
thanks, mostly LG. some nits around comments and request for a knob :)
================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:80
+
+ auto Task = [FIndex(FIndex), Path(Path.str()), Version(Version.str()),
+ ASTCtx(std::move(ASTCtx)),
----------------
let's leave a comment here saying that `FIndex` outlives the
`UpdateIndexCallbacks`.
================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:83
+ CanonIncludes(CanonIncludes)]() mutable {
+ FIndex->updatePreamble(Path, Version, ASTCtx.getASTContext(),
+ ASTCtx.getPreprocessor(), *CanonIncludes);
----------------
can you add a `trace::Span Tracer("PreambleIndexing");` here
================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:87
- if (FIndex)
- FIndex->updatePreamble(Path, Version, Ctx, PP, CanonIncludes);
+ if (Tasks) {
+ Tasks->runAsync("Preamble indexing for:" + Path + Version,
----------------
can you also introduce a `bool AsyncPreambleIndexing = false;` into
`ClangdServer::Options` struct and pass it into `UpdateIndexCallbacks` and make
this conditional on that? after testing it for couple weeks, we can make it the
default.
================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:211
IndexTasks.emplace();
+
// Pass a callback into `WorkScheduler` to extract symbols from a newly
----------------
nit: let's revert this change
================
Comment at: clang-tools-extra/clangd/Preamble.cpp:105
void AfterExecute(CompilerInstance &CI) override {
- if (ParsedCallback) {
- trace::Span Tracer("Running PreambleCallback");
- ParsedCallback(CI.getASTContext(), CI.getPreprocessor(), CanonIncludes);
+ // Set PrecompilePreambleConsumer/PCHGenerator to null.
+ if (CI.getASTReader()) {
----------------
the comments are still just talking about what the code is already doing, and
not "why".
```
// ASTContext and CompilerInstance can keep references to ASTConsumer
(PCHGenerator in case of preamble builds).
// These references will become dangling after preamble build finishes, even if
they didn't we don't want operations on the preamble to mutate PCH afterwards.
// So clear those references explicitly here.
```
================
Comment at: clang-tools-extra/clangd/Preamble.cpp:696
+ auto Ctx = CapturedInfo.takeLife();
+ // Set the FileManager VFS to consuming FS.
+ auto StatCacheFS = Result->StatCache->getConsumingFS(VFS);
----------------
again comment is talking about "what" the code does rather than "why", what
about:
```
// Stat cache is thread safe only when there are no producers. Hence change the
VFS underneath to a consuming fs.
```
================
Comment at: clang-tools-extra/clangd/Preamble.cpp:698
+ auto StatCacheFS = Result->StatCache->getConsumingFS(VFS);
+ Ctx->getFileManager().setVirtualFileSystem(StatCacheFS);
+ // While extending the life of FileMgr and VFS, StatCache should also be
----------------
nit: std::move(StatCacheFS) or just inline the initializer
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D148088/new/
https://reviews.llvm.org/D148088
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
