[PATCH] D47272: [clangd] Build index on preamble changes instead of the AST changes

[Eric Liu via Phabricator via cfe-commits]

ioeric accepted this revision.
ioeric added a comment.
This revision is now accepted and ready to land.

lg. Thanks for adding the test!



================
Comment at: clangd/index/FileIndex.cpp:37
+  std::vector<Decl *> TopLevelDecls(
+      AST.getTranslationUnitDecl()->decls().begin(),
+      AST.getTranslationUnitDecl()->decls().end());
----------------
ilya-biryukov wrote:
> ioeric wrote:
> > ilya-biryukov wrote:
> > > ioeric wrote:
> > > > Would this give us decls in namespaces? It seems that `AST.getTranslationUnitDecl()->decls()` are only decls that are immediately inside the TU context. 
> > > I'll double check and add a test for that, but I think the indexer visits the namespaces that we provide, I'm not sure if we have tests though.
> > Oh, right! In that case, it seems that a single TU decl which contains all interesting decls would be sufficient? Is it possible that preamble doesn't have all available top level decls in TU decl? This seems to be possible for ParsedAST, which is why we used `getTopLevelDecls`, I think?
> > In that case, it seems that a single TU decl which contains all interesting decls would be sufficient?
> I've tried doing the TU decl only and it does not work, because `indexTopLevelDecl` checks that location of decl that you pass to it is valid xD. Obviously, that's not the case for the TU decl. But it does visit the namespaces recursively, new test also checks for that, so we should be fine with the current code.
> 
> > This seems to be possible for ParsedAST, which is why we used getTopLevelDecls, I think?
> `getTopLevelDecls` was copied from the `ASTUnit` without much thought.
> IIRC, it allows less deserialization from preamble when traversing the AST for things like "go to def", etc. This is definitely not something we're optimizing for here, since we run on the preamble AST itself.
> Now that I think about it, I don't think we need to store top-level decls of preamble at all for any of our features...
Sounds good. Thanks for the clarification!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D47272