[PATCH] D67720: [clangd] Add semantic selection to ClangdLSPServer.

[UTKARSH SAXENA via Phabricator via cfe-commits]

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