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