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 
 - responses will be (much) bigger, which we know some clients don't deal well 
 - 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 

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 

(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 
Maybe reference the bug here.

  rG LLVM Github Monorepo


cfe-commits mailing list

Reply via email to