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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

mostly good, a few nits.



================
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:
> 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.
yes, it is correct now. maybe I read the wrong snapshot before.


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:136
+SelectionRange render(const std::vector<Range> &Ranges) {
+  if (Ranges.empty()) {
+    return {};
----------------
nit: remove the {}, in LLVM we prefer no {} for a single-statement body.


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1169
+        Result.emplace_back(render(std::move(*Ranges)));
+        return Reply(std::move(Result));
+      });
----------------
does `Reply({render(std::move(*Ranges))});` work?


================
Comment at: clang-tools-extra/clangd/Protocol.h:422
+  /// textDocument.selectionRange
+  bool SemanticSelection = false;
+
----------------
I think we could drop this field now, it is not used, as we always return `selectionRangeProvider:true`.


================
Comment at: clang-tools-extra/clangd/Protocol.h:1241
+  /**
+   * The [range](#Range) of this selection range.
+   */
----------------
nit: please remove the markdown elements around the `range` in the comment.


================
Comment at: clang-tools-extra/clangd/Protocol.h:1251
+  SelectionRange() = default;
+  SelectionRange(SelectionRange &&) = default;
+};
----------------
Are these constructors needed?


================
Comment at: clang-tools-extra/clangd/Protocol.h:1248
+   */
+  llvm::Optional<std::unique_ptr<SelectionRange>> parent;
+
----------------
usaxena95 wrote:
> 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. 
ah, good point, I see. We could just use `std::unique_ptr<SelectionRange> parent;`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67720/new/

https://reviews.llvm.org/D67720