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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
