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
+  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.

  rG LLVM Github Monorepo


cfe-commits mailing list

Reply via email to