[PATCH] D66637: [clangd] Support multifile edits as output of Tweaks

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:644
+          assert(It.second.Edits && "TextEdits hasn't been generated?");
+          if (auto Draft = DraftMgr.getDraft(It.first())) {
+            llvm::StringRef Contents = *Draft;
----------------
ilya-biryukov wrote:
> This callback is called asynchronously and the version inside `DraftMgr` may be newer than the one we used to calculate the original offsets inside replacements at this point.
> 
> We should not rely on `DraftMgr`, doing conversions on the source buffers inside `SourceManager` is probably the most reliable option we have.
I think you've misunderstood what this code is doing (it needs comments!)

The SourceManager contains the code that we've calculated the edits against, by definition. So checking against it does nothing.

When we send the edits to the client, it's going to apply them to the file if it's not open, but to the dirty buffer if it is open.
If the dirty buffer has different contents than what we saw (and calculated edits against) then the edits will be wrong - that's what this code guards against.

So looking at the latest available content in the draftmgr (at the end of the request, not the start!) is the right thing here, I think.


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:646
+            llvm::StringRef Contents = *Draft;
+            if (llvm::SHA1::hash(llvm::makeArrayRef(Contents.bytes_begin(),
+                                                    Contents.size())) !=
----------------
We're comparing here content in the source manager (which was read as bytes from disk, and converted to utf8) to that from LSP (which was sent as unicode text by the editor, and converted to utf8).

Are editors going to preserve the actual line endings from disk when sending LSP, or are they going to use the "array-of-lines" from their editor's native model? (e.g. join(lines, '\n'))

If the latter, we'd need to normalize line endings, possibly strip trailing line endings, etc.
I ask because clangd-vim definitely has an "array-of-lines" model :-)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66637/new/

https://reviews.llvm.org/D66637