usaxena95 added a comment.
> 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).
Yeah. So should I remove the client capability since we do not use it and just
return bool as now?
================
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 "
----------------
hokein wrote:
> 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?
We do right ?
If the size != 1 then we just return an error.
================
Comment at: clang-tools-extra/clangd/Protocol.h:1248
+ */
+ llvm::Optional<std::unique_ptr<SelectionRange>> parent;
+
----------------
hokein wrote:
> I think we can simplify the code further, using
> `llvm::Optional<SelectionRange>` should be enough, the parent is null for the
> outer-most range.
I don't think it is possible to do that since the type (SelectionRange) would
be incomplete at that point. For example size of this class cannot be computed.
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
