[PATCH] D76725: [clangd] Build ASTs only with fresh preambles or after building a new preamble

[Kadir Cetinkaya via Phabricator via cfe-commits]

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