kadircet added a comment.
preambleReady part is a little bit different than you've described, it looks
more like:
ASTWorker::preambleReady(): enqueue({
if(preamble!=lastPreamble) {
lastPreamble = preamble
cache.get(); // force next read to use this preamble
}
build ast
publish ast.diagnostics
if (inputs == currentInputs) cache.put(ast)
})
also the last 3 steps are on a different function called `generateDiagnostics`
since the code is a little bit long.
it immediately follows the `preambleReady`(now named `updatePreamble`).
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:17
+// An update request changes latest inputs to ensure any subsequent read sees
+// the version of the file they were requested. In addition to that it might
+// result in publishing diagnostics.
----------------
sammccall wrote:
> You need to mention "building an AST" here, as you reference it below.
just saying "it will issue a build for new inputs", which is explained in
details in the next paragraph.
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:29
+// modification to relevant part of the current file. Such a preamble is called
+// usable.
+// In the presence of stale(non-usable) preambles, ASTWorker won't publish
----------------
sammccall wrote:
> I don't think "usable" is a good name for this, because we *use* preambles
> that are not "usable".
>
> I think "compatible" or "up-to-date" or "fresh" or so would have enough
> semantic distance to make this less confusing.
using compatible.
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:341
SynchronizedTUStatus &Status;
+ ASTWorker &AW;
};
----------------
sammccall wrote:
> Please don't use initialisms like this for members, they're hard to follow
> out of context.
> I'd suggest ASTPeer (and PreamblePeer) or just Peer, to indicate these
> threads are tightly-coupled partners.
PW was for PeerWorker :P
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:554
+ LatestBuild->Version, Inputs.Version, FileName);
+ // We still notify the ASTWorker to make sure diagnostics are updated to
+ // the latest version of the file without requiring user update.
----------------
sammccall wrote:
> This is going to trigger a rebuild of the fileindex - is it really necessary?
astworker won't trigger that if inputs are the same.
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:630
+ WantDiagnostics WantDiags) {
+ std::string TaskName = llvm::formatv("Preamble Update ({0})", PI.Version);
+ // Store preamble and build diagnostics with new preamble if requested.
----------------
sammccall wrote:
> "Golden AST from preamble"?
> Current text is not terribly descriptive...
this is not necessarily a golden ast now. just changing it to "Build AST"
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:675
+ assert(LatestPreamble);
+ assert(isPreambleUsable(*LatestPreamble, Inputs, FileName, *Invocation));
+
----------------
sammccall wrote:
> this isn't a safe assert - it does IO and could transiently become true/false
right, thanks for catching it!
the one above is also not true (`assert(LatestPreamble)`) as preamble build
might've failed. I suppose we would still want to move forward so that we can
surface diagnostics saying `why it failed`.
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
