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