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

[Haojian Wu via Phabricator via cfe-commits]

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