simark marked 4 inline comments as done.
simark added inline comments.
================
Comment at: clangd/ClangdLSPServer.cpp:164
+ if (!Contents) {
+ log(llvm::toString(Contents.takeError()));
+ return;
----------------
ilya-biryukov wrote:
> We should signal an error to the client by calling `replyError`
`textDocument/didChange` is a jsonrpc notification, not request, so we can't
send back an error.
================
Comment at: clangd/DraftStore.cpp:106
+ } else {
+ NewContents = Change.text;
+ }
----------------
ilya-biryukov wrote:
> It is impossible to mix full content changes with incremental range changes,
> right?
>
> I suggest handling the full content change as a separate case at the start of
> the function:
> ```
> if (Changes.size() == 1 && !Changes[0].range) {
> Contents = std::move(Change.text);
> return Contents;
> }
>
> for (auto &Change : Changes) {
> if (!Change.range)
> return make_error("Full change in the middle of incremental changes");
> }
> ```
>
I'd say it is unlikely and there's probably no reason to do it, but the way the
data structure is allows it.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44272
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
