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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
