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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Thanks for addressing the comments. I have just one comment left, about the LSP versions and sanity-checking.



================
Comment at: clangd/DraftStore.h:36
   /// Replace contents of the draft for \p File with \p Contents.
-  void updateDraft(PathRef File, StringRef Contents);
+  void addDraft(PathRef File, StringRef Contents);
+
----------------
ilya-biryukov wrote:
> simark wrote:
> > ilya-biryukov wrote:
> > > Could we add versions from LSP's `VersionedTextDocumentIdentifier` here and make the relevant sanity checks?
> > > Applying diffs to the wrong version will cause everything to fall apart, so we should detect this error and signal it to the client as soon as possible.
> > I agree that applying diffs to the wrong version will break basically everything, but even if we detect a version mismatch, I don't see what we could do, since we don't have a mean to report the error to the client.  The only thing we could do is log it (which we already do.
> If we couldn't apply a diff, we should return errors from all future operations on this file until it gets closed and reopened. Otherwise clangd and the editor would see inconsistent contents for the file and results provided by clangd would be unreliable.
> The errors from any actions on the invalid file would actually be visible to the users.
> 
> The simplest way to achieve that is to remove the file from `DraftStore` and `ClangdServer` on errors from `updateDraft`.
> This will give "calling action on non-tracked file" errors for future operations and the actual root cause can be figured out from the logs.
We still ignore version numbers from the LSP.
Is this another change that didn't get in?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44272