LLE utf8: one ANSI-aware width for LLE, not three - #757
Merged
Conversation
Three LLE call sites measured "display width of text that may contain escape
sequences" and each carried its own walk:
- completion_menu_renderer had the CORRECT one -- the ECMA-48 final-byte
rule and the canonical width table.
- completion_menu_state counted one column per BYTE, so a CJK menu entry
measured three columns instead of two and the menu laid out too wide.
- the prompt composer ended a sequence only on `m`, so `\033[K` or a cursor
movement left the rest of the prompt measuring as zero width.
The correct one is promoted to lle_utf8_visible_width, next to the
lle_utf8_string_width primitive it builds on, and the other two become calls.
No escape walk remains in any of the three files.
This is the LLE half of the same consolidation the display subsystem got. The
two layers deliberately keep one implementation EACH rather than sharing a
single function: display depends on LLE and not the reverse, so a shared one
would have to live in LLE and be reached across the seam by display, or in
display and invert the dependency. Two canonical answers, one per layer, is
the correct outcome under that constraint -- and it is six fewer than there
were this morning.
tests/lle/unit/test_char_width.c covers the shared function on the terminators
that broke the individual copies -- cursor hide/show ending in `l` and `h`,
erase-line `K`, and `\033[3~` whose final byte is not a letter -- plus width
from the table rather than a byte count: a wide character inside escapes, an
em dash (one column, which the composer's guess made two), and a base plus
combining mark.
The test derives every length with strlen. An earlier version hand-counted one
and passed a length two bytes past the end of the literal, reading whatever
followed it; the failure looked exactly like a width bug in the code.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Third slice of duplicate-path item 6 — the LLE half.
Three walks, three answers
completion_menu_renderercompletion_menu_statem—\033[Kor a cursor movement left the rest of the prompt measuring zeroThe correct one is promoted to
lle_utf8_visible_width, beside thelle_utf8_string_widthprimitive it builds on. The other two become calls. No escape walk remains in any of the three files.Why two canonical functions, not one
Display got the same consolidation in #755/#756. These two layers deliberately keep one implementation each rather than sharing: display depends on LLE and not the reverse, so a single shared function would either live in LLE and be reached across the seam by display, or live in display and invert the dependency.
Two canonical answers — one per layer — is the correct outcome under that constraint. It is also six fewer than there were this morning.
Verification
tests/lle/unit/test_char_width.ccovers the shared function on exactly the terminators that broke the individual copies — cursor hide/show (l,h), erase-lineK, and\033[3~whose final byte is not a letter — plus width from the table rather than a byte count: a wide character inside escapes, an em dash (one column, which the composer's guess made two), and a base plus combining mark.Full suite 197/197. ASan 65/65. Net −128 lines across the three files.
One note on the test: it derives every length with
strlen. An earlier version hand-counted one and passed a length two bytes past the end of the literal, reading whatever followed — and the failure looked exactly like a width bug in the code rather than in the test.