hokein added a comment.
In D67720#1680460 <https://reviews.llvm.org/D67720#1680460>, @usaxena95 wrote:
> I am not sure adding client capability is useful here. I have not used the
> client capability for selectionRange anywhere and I think we can remove it.
> WDYT ?
The `SelectionRangeClientCapabilities` determines what should the LSP server
send the client, if it is true, clangd should send
`SelectionRangeRegistrationOptions`. But looking at the current specification,
it doesn't seem to add too much value. I think we can just simplify return a
bool for now (as you did in this patch).
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1131
+ Callback<std::vector<SelectionRange>> Reply) {
+ if (Params.positions.size() != 1) {
+ elog("{0} positions provided to SelectionRange. Supports exactly one "
----------------
usaxena95 wrote:
> ilya-biryukov wrote:
> > hokein wrote:
> > > maybe add an `assert(!Params.positions.empty())`. I think we should not
> > > run into this case.
> > But `Params` comes to clangd over LSP, right?
> > That means `assert` can fire in case of bad inputs over LSP to clangd.
> > Bad inputs over LSP should never crash clangd.
> Yes this comes from the client and can be a bad input. We should just return
> error and not crash in such case.
but the code still doesn't handle the `empty` case?
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:141
+ auto *Next = &Result.parent;
+ for (size_t I = 1; I < Ranges.size(); ++I) {
+ *Next = std::make_unique<SelectionRange>();
----------------
nit: use for range.
================
Comment at: clang-tools-extra/clangd/Protocol.h:1248
+ */
+ llvm::Optional<std::unique_ptr<SelectionRange>> parent;
+
----------------
I think we can simplify the code further, using
`llvm::Optional<SelectionRange>` should be enough, the parent is null for the
outer-most range.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67720/new/
https://reviews.llvm.org/D67720
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
