sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:967
+ URIForFile FileURI = Params.textDocument.uri;
+ Server->foldingRanges(
+ Params.textDocument.uri.file(),
----------------
one we fix the signature, you should just be able to pass the callback through.
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:969
+ Params.textDocument.uri.file(),
+ [FileURI, Reply = std::move(Reply)](
+ llvm::Expected<std::vector<FoldingRange>> Items) mutable {
----------------
unused capture (and variable)
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:90
Callback<llvm::json::Value>);
+ void onFoldingRange(const FoldingRangeParams &, Callback<llvm::json::Value>);
void onCodeAction(const CodeActionParams &, Callback<llvm::json::Value>);
----------------
this should return (as callback) vector<FoldingRange>.
You got unlucky: it's sandwiched between onDocumentSymbol and onCodeAction
which both have to be dynamically typed as they return different structures
depending on capabilities.
================
Comment at: clang-tools-extra/clangd/FindSymbols.cpp:281
+ Range.startCharacter = Symbol.range.start.character;
+ Range.endLine = Symbol.range.end.line;
+ Range.endCharacter = Symbol.range.end.character;
----------------
How useful are folding ranges when symbols start/end on the same line? Do we
really want to emit them?
================
Comment at: clang-tools-extra/clangd/FindSymbols.cpp:297
+llvm::Expected<std::vector<FoldingRange>> getFoldingRanges(ParsedAST &AST) {
+ auto DocumentSymbols = collectDocSymbols(AST);
+ std::vector<FoldingRange> Result;
----------------
I'm not sure whether this is the right foundation, vs RecursiveASTVisitor.
How would we generalize to compoundstmts etc without RAV, and if we do use RAV
for those, why would we still use getDocSymbols for anythnig?
================
Comment at: clang-tools-extra/clangd/FindSymbols.h:50
+/// Retrieves folding ranges using Document Symbols in the "main file" section
+/// of given AST.
----------------
"using Document Symbols" seems like an implementation detail here, I'd keep it
out of the first line at least.
Maybe separate out explicitly in a second line: Currently this includes the
ranges of multi-line symbols (functions etc)
================
Comment at: clang-tools-extra/clangd/FindSymbols.h:52
+/// of given AST.
+llvm::Expected<std::vector<FoldingRange>> getFoldingRanges(ParsedAST &AST);
+
----------------
This seems like a surprising place to find this function - it only really makes
sense when considering the current implementation (which will presumably grow).
I think it'd be a more natural fit in SemanticSelection, maybe.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82436/new/
https://reviews.llvm.org/D82436
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
