[PATCH] D88553: [clangd] Start using SyntaxTrees for folding ranges feature

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:40
 
-// Recursively collects FoldingRange from a symbol and its children.
-void collectFoldingRanges(DocumentSymbol Symbol,
-                          std::vector<FoldingRange> &Result) {
+llvm::Optional<FoldingRange> toFoldingRange(SourceRange SR,
+                                            const SourceManager &SM) {
----------------
Here you're looking at positions but ignoring file ID, which is always a reason to be cautious...

We've dropped macro IDs, and we're traversing only the syntax tree for this file, but we could still end up in the wrong file:
```
void foo() {
  #include "SomeTablegen.inc"
}
```

We need to verify both start/end are in the main file.

I'd suggest passing in the main FileID as a param here, and decomposing the start/end locations into (FileID, Offset), and bailing out if either FID doesn't match.
This subsumes the macro check (since macro expansions have their own FileIDs) though you might want to keep it to be explicit, as we likely want to support some cases later.

As a bonus, then you get to use SourceManager::get{Line,Column}Number(FileID, Offset) instead of the "spelling" variants that are slightly confusing as we've already established we have a file location already.


================
Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:60
+               *RBrace = cast_or_null<syntax::Leaf>(
+                   Stmt->findChild(syntax::NodeRole::CloseParen));
+    if (!LBrace || !RBrace)
----------------
strictly this should probably be findLastChild both semantically and for performance... but that doesn't exist, because it's a single-linked list for now.

Not a big deal in practice, but we should fix this (and add STL iterators!)


================
Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:158
+  const auto *SyntaxTree =
+      syntax::buildSyntaxTree(A, *AST.getASTContext().getTranslationUnitDecl());
+  return collectFoldingRanges(SyntaxTree, AST.getSourceManager());
----------------
this actually does the right thing (traverses the ASTContext's registered roots rather than the TUDecl itself), but... what a misleading API. We should fix that to take ASTContext instead...

(Not in this patch, maybe a followup?)


================
Comment at: clang/include/clang/Tooling/Syntax/Tree.h:175
+  const Node *findChild(NodeRole R) const {
+    return const_cast<syntax::Tree *>(this)->findChild(R);
+  }
----------------
I think it's more conventional to implement the const variant and use const_cast for the non-const variant, so the compiler will verify that the function implementation is const, and the cast is just adding "I know if I'm not const then the returned object isn't either".

By implementing the *non-const* variant, the compiler doesn't help you verify any part of the const contract.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88553