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?

  rG LLVM Github Monorepo



cfe-commits mailing list

Reply via email to