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