[PATCH] D47331: [clangd] Remove accessors for top-level decls from preamble

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/ClangdUnit.h:81
 
   ASTContext &getASTContext();
   const ASTContext &getASTContext() const;
----------------
ioeric wrote:
> IIUC, `ASTContext` in a `ParsedAST` may not contain information from preambles and thus may give an incomplete AST. If so, I think we should make this more explicit in the class level to make sure this is not misused (e.g. in case the incomplete context is used to build dynamic index).
Added a small doc comment about it. Let me know if you feel we need to elaborate more on this.
I don't think anything changed wrt to this behavior in this patch, apart from the fact that we used to deserialize more stuff most of the time before so this wan't as visible.


================
Comment at: unittests/clangd/TestTU.cpp:77
   const NamedDecl *Result = nullptr;
-  for (const Decl *D : AST.getTopLevelDecls()) {
+  for (const Decl *D : AST.getLocalTopLevelDecls()) {
     auto *ND = dyn_cast<NamedDecl>(D);
----------------
sammccall wrote:
> isn't this incorrect? Or should be "findDeclInMainFile" or similar?
> 
> I'd think this would conflict with your other patch, which uses this to test the boost of things that come from the header.
Thanks for spotting that, I would also expect this to fail in the new patch.
I'll look into rewriting this helper.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D47331