[PATCH] D50847: [clangd] Add callbacks on parsed AST in addition to parsed preambles

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/TUScheduler.cpp:406
     // We only need to build the AST if diagnostics were requested.
     if (WantDiags == WantDiagnostics::No)
       return;
----------------
hokein wrote:
> The AST might not get built if `WantDiags::No`, and will be built in `runWithAST`.
I suggest we keep it a known limitation and not rebuild the index in that case.
The original intent of `WantDiags::No` was to update the contents of the file, so that upcoming request can see the new contents (e.g. this is a hack to avoid passing overriden contents of the file in the request itself).
`WantDiags::No` is always followed by an actual request that wants the diagnostics (and will, therefore, build the AST and emit the callback).

Alternatively, we could unify the code paths building the AST. Would be a bit more intrusive change, but I guess it's worth it.


================
Comment at: clangd/TUScheduler.h:66
+  /// instead.
+  virtual void onMainAST(PathRef Path, ParsedAST &AST) = 0;
+};
----------------
hokein wrote:
> Does the callback get called every time we built an AST? clangd only has 3 idle ASTs, if the AST is not there, we rebuild it when needed (even the source code is not changed), and we will update the dynamic index, which seems unnecessary.
> 
> It may rarely happen, 3 idle ASTs  might cover most cases, I think? 
Currently we only call this when AST is built for diagnostics, so we will have only a single callback, even if the AST will be rebuilt multiple times because it was flushed out of the cash (as long as the file contents didn't change, of course).

So we do behave optimally in that case and I suggest we keep it this way


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50847