[PATCH] D82436: [clangd] Implement textDocument/foldingRange

[Sam McCall via Phabricator via cfe-commits]

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