This is an archive of the discontinued LLVM Phabricator instance.

Fix range length comparison in DraftStore::UpdateDraft when Unicode characters are removed from the document
ClosedPublic

Authored by DaanDeMeyer on Oct 22 2018, 1:42 PM.

Details

Summary

See http://lists.llvm.org/pipermail/clangd-dev/2018-October/000171.html for context.

I kept the error (instead of downgrading to a log message) since the range lengths differing does indicate either a bug in the client or server range calculation or the buffers being out of sync (which both seems serious enough to me to be an error). If any existing clients aside from VSCode break they should only break when accidentally typing a Unicode character which should only be a minor nuisance for a little while until the bug is fixed in the respective client.

Diff Detail

Event Timeline

DaanDeMeyer created this revision.Oct 22 2018, 1:42 PM
sammccall accepted this revision.Oct 23 2018, 3:00 AM

Thanks for the fix! Just comment nits.

clangd/DraftStore.cpp
85

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.

clangd/SourceCode.cpp
71

actually just moving this comment to the header would be great!

74

could you few simple assertions for this function (maybe ascii, BMP, astral) to unittests/clangd/SourceCodeTests.cpp?

clangd/SourceCode.h
26

I think it would be helpful to mention here (only in the comment!) that this is UTF-16 code units.

This revision is now accepted and ready to land.Oct 23 2018, 3:00 AM

Update diff according to comments.

This revision was automatically updated to reflect the committed changes.