sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Thanks for your patience!
This is very nice, only +100 lines on TUScheduler.cpp in the end!
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:224
+ std::vector<Diag> CIDiags, WantDiagnostics WantDiags) {
// Make possibly expensive copy while not holding the lock.
+ Request Req = {std::move(CI), std::move(PI), std::move(CIDiags), WantDiags,
----------------
comment is now obsolete, remove it and consider inlining `Req`
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:325
mutable std::condition_variable ReqCV; /* GUARDED_BY(Mutex) */
std::shared_ptr<const PreambleData> LatestBuild; /* GUARDED_BY(Mutex) */
----------------
this is no longer locked or concurrently accessed right?
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:378
+ /// Used to inform ASTWorker about a new preamble build by PreambleThread.
+ /// Diagnostics are only published through this callback, which ensures they
+ /// are always for newer versions of the file. As this callback gets called
in
----------------
nit: last sentence is a fragment, I think you want to swap the dot and comma:
...through this callback. This ensures... of the file, as the callback...
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:408
+
+ /// Returns the latest built preamble, might be null if no preamble has been
+ /// built or latest attempt resulted in a failure.
----------------
Unused, I think?
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:793
+ // Used to check whether we can update AST cache.
+ bool InputsAreTheSame =
+ std::tie(FileInputs->CompileCommand, FileInputs->Contents) ==
----------------
I'd suggest InputsAreLatest or something that hints at *why* they might differ
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:810
+ });
+ llvm::Optional<std::unique_ptr<ParsedAST>> AST = IdleASTs.take(this);
+ if (!AST) {
----------------
This is reusing the AST built for a read if the file (and preamble) hasn't
changed, right?
I think it's helpful to explain where teh cache data might be coming from.
IIRC this is the case we thought wasn't so important. It competes with a
different optimization: skipping clang-tidy, warning analysis, etc when
building ASTs because we know their diagnostics will never be used.
This might be a good place to leave a comment like "FIXME: maybe we're better
off never reusing this AST, so queued AST builds aren't required to produce
diagnostics" or so
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:862
+ // Stash the AST in the cache for further use if it was build for current
+ // inputs.
+ if (InputsAreTheSame) {
----------------
This explains the what but not the why.
(We may be building an older version of the source, as the queue raced ahead
while we were waiting on the preamble. In that case the queue can't reuse the
AST we built.)
Comment could go here or on InputsAreTheSame
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76725/new/
https://reviews.llvm.org/D76725
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
