expand/unexpand: fix display width of 3- and 4-byte UTF-8 characters #13352
Open
sylvestre wants to merge 4 commits into
Open
expand/unexpand: fix display width of 3- and 4-byte UTF-8 characters #13352sylvestre wants to merge 4 commits into
sylvestre wants to merge 4 commits into
Conversation
The UTF-8 sequence length was derived from char::from(b).len_utf8(), which uses the leading byte's value as a codepoint rather than decoding the sequence length from its bit pattern. This only happened to be correct for 2-byte characters; 3- and 4-byte characters were split into individual bytes and counted as one column each, so a following tab expanded to the wrong column. Should make test tests/expand/mb.sh pass
next_char_info derived the UTF-8 sequence length from char::from(b).len_utf8() (the leading byte's codepoint value, correct only for 2-byte sequences) and then used that byte count as the display width. So 3- and 4-byte characters were mis-measured, and every multibyte character advanced the column by its byte length instead of its display width, mis-aligning tab conversion. Derive the length from the leading byte and use UnicodeWidthChar for the width, matching GNU 9.11. Update the multibyte column test, which asserted the old byte-count behavior.
Merging this PR will improve performance by ×2.6
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Simulation | unexpand_large_file[10] |
282.3 ms | 107 ms | ×2.6 |
| ⚡ | Simulation | unexpand_many_lines[100000] |
134.7 ms | 51.2 ms | ×2.6 |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing sylvestre:expand-unexpand-mb-width (e607a91) with main (e646122)
Footnotes
-
46 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
sylvestre
force-pushed
the
expand-unexpand-mb-width
branch
from
82a054d to
f0138a1
Compare
Both utilities derived the UTF-8 sequence length from the leading byte and computed a character's display width identically. Extract that into a uucore::char_width helper and use it from both, removing the duplicated (and subtle, w.r.t. invalid UTF-8) logic. unicode-width is a direct, non-optional uucore dependency: os_display already pulls it into every uucore build, so gating the helper behind an optional-dep feature added no leanness and failed to activate on wasm.
The fast path that batches leading spaces/tabs was gated on --no-utf8, so the default (UTF-8) mode fell back to the per-byte loop, which the display-width fix made measurably slower on ASCII input. Leading whitespace is ASCII and independent of UTF-8 mode, so run the fast path there too, and add the missing lastcol == 0 guard so it does not apply when multiple tab stops cap conversion.
sylvestre
force-pushed
the
expand-unexpand-mb-width
branch
from
f0138a1 to
3c971f8
Compare
|
GNU testsuite comparison: |
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
1 participant
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.
No description provided.