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