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

[Eduardo Caldas via Phabricator via cfe-commits]

eduucaldas 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:
> kbobyrev wrote:
> > 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?
> Exactly. And bail out if both don't exist.
> 
> And this can be done on Tree, so it's trivial to add support for function calls etc (but feel free to keep the scope small)
I very much agree on all the points you made Sam.

I think the logic to get ranges from node roles should eventually live in the syntax trees library. We might want to hide some of these lower-level functions in the future, so it would be better to not rely on them. But it's no harm for now :).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88553