[PATCH] D53527: Fix range length comparison in DraftStore::UpdateDraft when Unicode characters are removed from the document

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Thanks for the fix! Just comment nits.



================
Comment at: clangd/DraftStore.cpp:85
+
+    // The difference between EndIndex and StartIndex gives the range length in
+    // bytes. However, the LSP range length is specified in UTF-16 code units
----------------
can we summarize this a bit?, e.g.
`// EndIndex and StartIndex are in bytes, but rangeLength is in UTF-16 units`
no need to explain exactly what the code is doing, just need to say why.


================
Comment at: clangd/SourceCode.cpp:70
 
-// Counts the number of UTF-16 code units needed to represent a string.
-// Like most strings in clangd, the input is UTF-8 encoded.
-static size_t utf16Len(StringRef U8) {
+// Counts the number of UTF-16 code units needed to represent a string (LSP
+// specifies string lengths in UTF-16 code units). Like most strings in clangd,
----------------
actually just moving this comment to the header would be great!


================
Comment at: clangd/SourceCode.cpp:73
+// the input is UTF-8 encoded.
+size_t lspLength(StringRef Code) {
   // A codepoint takes two UTF-16 code unit if it's astral (outside BMP).
----------------
could you few simple assertions for this function (maybe ascii, BMP, astral) to unittests/clangd/SourceCodeTests.cpp?


================
Comment at: clangd/SourceCode.h:26
 
+/// Calculate the length of Code according to the LSP specification.
+size_t lspLength(StringRef Code);
----------------
I think it would be helpful to mention here (only in the comment!) that this is UTF-16 code units.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53527