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