kadircet marked 16 inline comments as done.
kadircet added inline comments.
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:218
+ PreambleRequested.wait(Lock, [this] { return PreambleReq || Done; });
+ // No request means we are done.
+ if (!PreambleReq)
----------------
sammccall wrote:
> Why do we rebuild the preamble if stop is requested?
it was to be consistent with astworker, but i suppose it isn't necessary as
ASTWorker does that to make sure each LSP request gets a reply, and it can be
done with stale preambles.
Are there any other reasons for ASTWorker to empty the queue after stop?
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:295
+ std::shared_ptr<const PreambleData> OldPreamble =
+ Inputs.ForceRebuild ? std::shared_ptr<const PreambleData>()
+ : getLatestBuiltPreamble();
----------------
sammccall wrote:
> (nullptr should work here?)
right, it was copied over :/
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:312
+
+ mutable std::mutex Mutex;
+ bool Done = false;
----------------
sammccall wrote:
> Yikes, lots of state :-)
> I'd consider merging the two CVs - I find keeping the events separate and
> reasoning when you need notify_one vs _all doesn't seem to pay off, may just
> be the way I think about queues. Up to you.
i like them to be seperate as it makes naming easier :D but i agree that having
less state also makes reasoning easier.
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:600
Inputs, CompilerInvocationDiagConsumer, &CC1Args);
+ auto OldPreamble = PW.getLatestBuiltPreamble();
+ PW.requestBuild(Invocation.get(), Inputs);
----------------
sammccall wrote:
> this doesn't seem correct (maybe ok in this patch because of the blocking,
> but not in general). You're assuming the last available preamble is the one
> that the last AST was built with.
>
> I suppose you can't check the preamble of the current ParsedAST because it
> might not be cached, and you nevertheless want to skip rebuild if the
> diagnostics are going to be the same. I can't think of anything better than
> continuing to hold the shared_ptr for PreambleForLastBuiltAST or something
> like that.
right, this is just a "hack" to keep this change NFC.
in the follow-up patches i am planning to signal whether latest built preamble
is reusable for a given `ParseInputs`, and also signal what the AST should be
patched with.
diagnostics(ast) will only be built if preamble is re-usable.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76125/new/
https://reviews.llvm.org/D76125
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
