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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added inline comments.


================
Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:52-55
+    const syntax::Token *FirstToken = Tree->findFirstLeaf()->getToken(),
+                        *LastToken = Tree->findLastLeaf()->getToken();
+    assert(FirstToken->kind() == tok::TokenKind::l_brace);
+    assert(LastToken->kind() == tok::TokenKind::r_brace);
----------------
sammccall wrote:
> eduucaldas wrote:
> > Take a look at `clang/include/clang/Tooling/Syntax/Nodes.h`, syntax constructs usually have nice classes with accessors.
> > 
> > For instance `CompoundStatement` has the accessors `getLbrace` and `getRbrace` that seem to be exactly what you want.
> > 
> > However these might not give exactly the first leaf and last leaf in the case of syntactically incorrect code.
> I think we should treat all bracket-like things generically. Today this is just CompoundStmt, but we want to handle init lists, function calls, parens around for-loop conditions, template parameter and arg lists etc in the same way.
> 
> This sort of use is why the `OpenParen`/`CloseParen` NodeRoles are generic - we can have one set of logic to handle all of these. (No opinion on whether that should live here or in the syntax trees library, but putting it here for now seems fine).
> 
> So in the end I think checking the class name and then grabbing the braces by role (not kind) is the right thing here.
> We definitely want to avoid asserting that the code looks the way we expect though.
> So in the end I think checking the class name and then grabbing the braces by role (not kind) is the right thing here.
> We definitely want to avoid asserting that the code looks the way we expect though.

Can you elaborate a bit on how this would work? Is your proposal to iterate through `CompoundStatement` first-level children and grab the `OpenParen` + `CloseParen` roles?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88553