[PATCH] D101057: [clangd] Allow AST request without range

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:895
+          auto Node = DynTypedNode::create(
+                *Inputs->AST.getASTContext().getTranslationUnitDecl());
+          return CB(dumpAST(Node, Inputs->AST.getTokens(),
----------------
Ooh, there's a problem here...

TraverseDecl(TranslationUnitDecl) is going to deserialize the whole preamble and try to return the AST for every decl in every included header.
Apart from a huge amount of data, it's going to be extremely slow, we try to avoid ever doing this.

I think this should be observable in a unit test: if you set TestTU.Code and TestTU.HeaderCode, we want decls from the latter not to be visible in the dumped tree.

---

Instead, dumpAST should call DumpVisitor.TraverseAST(), which runs over the TraversalScope forest (i.e. all the top-level decls of the current file). But we still need the TranslationUnitDecl in order to have a single root.

Therefore I think we need this logic for "full tree" dumping:
```
DumpVisitor.traverseNode("declaration", TUDecl, [&] { DumpVisitor.TraverseAST() });
```

I think it's OK to keep the dumpAST signature as-is (i.e. pass a DynTypedNode with TUDecl in it as you're doing now) and just override TraverseTranslationUnitDecl with this special case. However both dumpAST() and the callsite should have a comment saying that TranslationUnitDecl is safe/has these special semantics, as that's not typical in clangd.

---

(The previous version of this patch had this problem too, it was just less obvious. Sorry for not catching it in the first round!)


================
Comment at: clang-tools-extra/clangd/ClangdServer.h:347
   /// Describe the AST subtree for a piece of code.
-  void getAST(PathRef File, Range R, Callback<llvm::Optional<ASTNode>> CB);
+  void getAST(PathRef File, const llvm::Optional<Range> &R,
+              Callback<llvm::Optional<ASTNode>> CB);
----------------
Nit: we tend to pass Range etc by value rather than const ref.
I actually have no idea why! But probably best to be consistent and if we clean this up we can do it everywhere at once.


================
Comment at: clang-tools-extra/clangd/Protocol.h:1728
   /// The highest-level node that entirely contains the range will be returned.
-  Range range;
+  /// If no range is given, the top-level translation unit node will be
+  /// returned.
----------------
nit: top-level->root
(We usually use "top-level" to mean children of TUDecl)


================
Comment at: clang-tools-extra/clangd/unittests/DumpASTTests.cpp:160
 
+TEST(DumpASTTests, NoRange) {
+  ParsedAST AST = TestTU::withCode("int x;").build();
----------------
as mentioned, this test should have decls in the header file too (HeaderCode, which is implicitly included) to make sure that we're not returning those


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101057/new/

https://reviews.llvm.org/D101057