simark added inline comments.
================ 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: > 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? The more I think about it, the less sure I am that this is the intended usage of the version. The spec doesn't even say what the initial version of a file should be, 0 or 1? Then, it just says that the version optionally contained in the `VersionedTextDocumentIdentifier` reflects the version of the document after having applied the edit. But I don't think we can predict and validate what that version should be. Most clients will probably just use a sequence number, but I guess they don't have to... Also, I initially assumed that having N changes in the `contentChanges` array would mean that the version number would be bumped by N. But now that I re-read it, there's really nothing that says it should behave like this. I think that we should just record the version number and treat it as opaque, so that we can use it later when sending text edits to the client, for example. The client can then verify that the edit is for the same version of the document that it has at the moment. Therefore, I don't think it would really be useful in this patch. 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
