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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
