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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

I think we're missing the client/server capability implementation in this patch, we should include them.



================
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 "
----------------
maybe add an `assert(!Params.positions.empty())`. I think we should not run into this case.


================
Comment at: clang-tools-extra/clangd/Protocol.h:1234
+
+struct SelectionRange {
+  // The semantic ranges for a position. Any range must contain all the previous
----------------
The data structures defined in `Protocol` are mirrored to LSP's. I think we should follow it for semantic selection as well (even though LSP use a wired structure, linked list).

so here, the structure should be like 

```
struct SelectionRange {
    /**
     * The [range](#Range) of this selection range.
     */
    Range range;
    /**
     * The parent selection range containing this range. Therefore `parent.range` must contain `this.range`.
     */
    llvm::Optional<SelectionRange> parent;
}
```

And we'd need to define a `render` function in `ClangLSPServer` to covert the `vector<Range>` to the LSP's `SelectionRange`.


================
Comment at: clang-tools-extra/clangd/Protocol.h:1241
+llvm::json::Value toJSON(const SelectionRange &);
+llvm::raw_ostream &operator<<(llvm::raw_ostream &, const SelectionRange &);
+
----------------
does this operator get used in this patch?


================
Comment at: clang-tools-extra/clangd/test/selection-range.test:4
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,"text":"void func() {\n int var1;\n int var2 = var1;\n}"}}}
+---
----------------
could we create a smaller test? just `void func() {}` is enough, I'd keep the lit test as small as possible (since this feature has been test in the unittest)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67720