Share the character count between core and mruby-string-ext - #7104
Merged
Conversation
`String#slice!` walked the string itself to turn a character index into a byte offset, in `str_char_to_byte_offset()` and `str_chars_to_byte_len()`. Both handed `mrb_utf8len()` an end of `p + byte_len - byte_offset`, which is the end of the string only while `byte_offset` is zero: every character consumed moves that end one character closer to the start. A character reaching past the moved end is measured as truncated, which `mrb_utf8len()` reports as a single byte, so the conversion answers an offset that lands inside a character: ```ruby "あいうえお".slice!(3, 2) #=> "\xE3\x81" (expected "えお") "あいう".slice!(1..2) #=> "い\xE3" (expected "いう") "あいう".slice!(-1) #=> "\xE3" (expected "う") ``` The bytes the receiver keeps are the ones the result did not take, so the receiver is broken as well: the first line above leaves it holding `"あいう\x88お"`. `mrb_str_char_to_byte()` does this conversion in core and is what `String#[]` goes through, which is why `"あいうえお"[3, 2]` answers `"えお"` where `slice!` does not. Call it for both offsets and drop the two static helpers. It is declared outside the `MRB_UTF8_STRING` guard and is the identity on a build without UTF-8 support, so the guarded branch goes with them.
`String#slice!(str)` searches with `mrb_str_index()`, which answers a byte
offset, and turned that into a character index by counting the characters
of `mrb_str_substr(mrb, self, 0, pos)`. `mrb_str_substr()` reads its
arguments as character positions, so the byte offset arrived there as a
character count and the substring it cut is longer than the part before the
match. The index that comes out is too large:
```ruby
"あいう".slice!("い") #=> "" (expected "い")
```
`"い"` starts at byte 3, `mrb_str_substr()` takes the 3 as three characters
and hands back the whole receiver, and the match is reported at character
3 rather than 1. The length is then clamped against the receiver, which
leaves nothing to cut.
`mrb_str_byte_to_char()` is the conversion this wants, and it also settles
what to do when the offset is not the start of a character: the search runs
over bytes and can land inside one, which it reports as -1. CRuby finds no
match in that position, so answer nil for it.
`utf8_strlen()` counts the characters of a string the way the string itself is indexed: a binary or single-byte string has one character per byte, everything else is measured as UTF-8. It belongs with `mrb_str_char_to_byte()` and `mrb_str_byte_to_char()`, which answer positions under that same rule, but unlike them it was static and reachable only through the `RSTRING_CHAR_LEN` macro. mruby-string-ext carries its own copy of it for want of a shared one. Give the count the shape the two conversions already have: `mrb_str_char_len(mrb, str)`, declared in `internal.h` outside the `MRB_UTF8_STRING` guard, with a definition on the other side of the guard that returns the byte length. Both `RSTRING_CHAR_LEN` definitions go away and the six uses call the function. The `mrb` argument goes unused here as it does in the two conversions. It is in the signature so the three read alike where they are called.
`str_char_count()` is `mrb_str_char_len()` written a second time: a binary or single-byte string counts one character per byte, anything else goes through `mrb_utf8_strlen()`. Call the core helper and drop the copy. The helper is declared outside the `MRB_UTF8_STRING` guard and returns the byte length on a build without UTF-8 support, so `String#slice!` loses the guarded branch it chose its length with. `ljust`, `rjust` and `center` were already calling the copy from outside any guard. The core helper also keeps what it learns: a string whose byte length and character count agree is marked single-byte, so the next question about it is answered without walking the bytes again. The copy threw that away.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR adds the internal ChangesString character length handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Core counts the characters of a string in
utf8_strlen(), reached through theRSTRING_CHAR_LENmacro. mruby-string-ext counts them again instr_char_count(), with the same rule in the same order: a binary orsingle-byte string has one character per byte, anything else goes through
mrb_utf8_strlen().The count is the last of the three questions asked about character positions
that had no shared answer.
mrb_str_char_to_byte()andmrb_str_byte_to_char()became internal API in #7097; this does the same for the count, so the three
sit together in
internal.hand read alike where they are called.mrb_str_char_len(mrb, str)utf8_strlen()promoted, declared outside theMRB_UTF8_STRINGguard with abyte-length definition on the other side of it, exactly as the two conversions
are. Both
RSTRING_CHAR_LENdefinitions go away and the six uses in core callthe function.
Nothing in core changes behavior: the same function is doing the same work
under a different name.
mruby-string-ext
str_char_count()is deleted and its eight call sites, inljust,rjust,centerandslice!, callmrb_str_char_len(). Because the helper isavailable on a build without UTF-8 support,
String#slice!loses one#ifdef MRB_UTF8_STRING.One difference comes with the move: core marks a string single-byte once it
finds its byte length and character count agree, so a later question about the
same string is answered without walking it again. The copy in the gem dropped
that.
Base
Branched off #7103, which is still open, because both touch
String#slice!.The first two commits here are that PR and drop out once it merges.
Verified
rake teston a full-core build (MRB_UTF8_STRINGthrough mruby-encoding):2237 tests, all green
rake teston the default gembox (noMRB_UTF8_STRING): 2053 tests, allgreen
prek run --all-filespasses, except thatmarkdownlintcould not installlocally (npm engine mismatch); no Markdown is touched here
Summary by CodeRabbit
String#slice!, including character-based ranges, negative indexes, and multibyte matches.