sammccall 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); ---------------- 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) ================ Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:771 } - Leaf *getLbrace(); + Leaf *getLbrace() const; /// FIXME: use custom iterator instead of 'vector'. ---------------- This doesn't look right: now a const method grants mutable access to the child. I think we need both overloads :-( (Fortunately this is to be tablegen'd one day...) 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
