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

Thu Jul 2 04:48:09 PDT 2020

sammccall added a comment.

I think this looks reasonable, but I'd like to make sure we have a plan going forward otherwise the behavior/assumptions tend to calcify.
I think RAV and adding other region types are clear, but maybe we should discuss lines/filtering behavior offline a bit.

================
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;
----------------
kbobyrev wrote:
> 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?
Well, I don't think "the protocol allows regions within a line, therefore we must emit them" is a great argument. You mention a valid use case, but you haven't *described* any use cases - what specific C++ intra-line region is the user likely to fold in a way that's useful?

I think that it's hard to point to concrete benefits, but the costs are clear enough:
 - responses will be (much) bigger, which we know some clients don't deal well with
 - clients are not likely to do any smart filtering, so I think this will lead to performance/ux problems in practice (especially if most implementations only support folding blocks etc)
 - it makes decisions on what folding regions to emit harder, e.g. should we fold function calls, not fold them, or fold them only if start/end are on different lines?
 - we need two different modes, to handle clients that support line-folds vs clients that support char-folds. (If we only emit multi-line folds we could also only emit the inner (or the outer) fold for a pair of lines, and the result would be suitable for both types of editors)

> when there is a limit
It's pretty important that this feature behaves consistently, for it to be useful. If we're going to only-sometimes enable folding of certain constructs, it better be for a reason that's pretty obvious to the user (I believe single vs multiline qualifies, but "the document complexity exceeds X" certainly doesn't.) For this reason I don't think we should implement that capabilities feature (or should do it in some totally naive and unobtrusive way, like truncation)

================
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;
----------------
kbobyrev wrote:
> 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?
That sounds fair enough - it is very simple. I'd suggest leaving a slightly more specific fixme: like "getDocumentSymbols() is conveniently available but limited (e.g. doesn't yield blocks inside functions). Replace this with a more general RecursiveASTVisitor implementation instead."

================
Comment at: clang-tools-extra/clangd/Protocol.h:1519
+/// Stores information about a region of code that can be folded.
+/// FIXME(kirillbobyrev): Implement FoldingRangeClientCapabilities.
+struct FoldingRange {
----------------
This doesn't really specify what is to be fixed and why.
Vs FIXME: add FoldingRangeClientCapabilities so we can support per-line-folding editors.

(wouldn't this fixme belong on the *request* rather than the response?)

================
Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:100

+// FIXME(kirillbobyrev): Collect commenets, PP definitions and other code
+// regions (e.g. public/private sections of classes, control flow statement
----------------
nit: comments
I think PP conditional regions and includes are maybe more relevant than definitions?
Maybe reference the bug here.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82436