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