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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

The interfaces look mostly good to me.



================
Comment at: clangd/ClangdServer.cpp:73
+
+  void onPreambleAST(PathRef Path, ASTContext &Ctx,
+                     std::shared_ptr<clang::Preprocessor> PP) override {
----------------
I think `Ctx` should be a `pointer` which gives us a way (passing a nullptr) to clean up the `FileIndex`, the same as `ParsedAST` below.

And I discover that we don't cleanup dynamic index when a file is being closed, is it expected or a bug?


================
Comment at: clangd/TUScheduler.cpp:406
     // We only need to build the AST if diagnostics were requested.
     if (WantDiags == WantDiagnostics::No)
       return;
----------------
ilya-biryukov wrote:
> 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.
I see, SG, thanks!


================
Comment at: clangd/TUScheduler.h:66
+  /// instead.
+  virtual void onMainAST(PathRef Path, ParsedAST &AST) = 0;
+};
----------------
ioeric wrote:
> ilya-biryukov wrote:
> > 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
> is there any overlap between preamble AST and main AST? If so, could you elaborate in the documentation? 
> 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).

It sounds reasonable, thanks for the explanation. Could you clarify it in the comment?

> is there any overlap between preamble AST and main AST? If so, could you elaborate in the documentation?

AFAIK, both of them contain different `TopLevelDecls`, but it is still possible to get the `Decl*` (declared in header) when visiting the main AST (e.g. when visiting the `void foo() {}` definition in `MainAST`, we can still get the `void foo();` declaration in `PreambleAST`). 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50847