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