[PATCH] D44272: [clangd] Support incremental document syncing

[Simon Marchi via Phabricator via cfe-commits]

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