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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added inline comments.


================
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;
----------------
sammccall wrote:
> How useful are folding ranges when symbols start/end on the same line? Do we really want to emit them?
I think they are useful from the LSP perspective, if characters didn't matter in the ranges they wouldn't be included in the protocol at all and there wouldn't be any way for clients to specify their preferences.

I don't think it's a common use case, but I do think it's a totally valid one. Maybe those should be the first candidates for excluding when there is a limit and maybe we could completely cut them for performance. But from the LSP perspective it seems logical to have those.

What do you think?


================
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;
----------------
sammccall wrote:
> 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?
Sure, I agree. The thing is I wanted to do folding ranges in an incremental way while gradually building the right foundations. RAV + TokenBuffers certainly seems right for the final implementation but it'd be much more code and tests and I didn't want to push a big patch since it is harder to review and also harder to feel the real progress. Pushing this patch allows me to parallelize the work, too, since there are several improvements on the LSP side which wouldn't touch implementation.

I think it'd be better to leave the current implementation and simply replace it in the next patch, do you see any reasons why this wouldn't be a good option?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82436/new/

https://reviews.llvm.org/D82436