[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:
> 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)
As discussed online, we should emit single-line ranges.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82436