Share UTF-8 byte lengths and character heads between core and mruby-regexp - #7109
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change centralizes UTF-8 length and boundary handling in shared internal helpers. Regexp processing uses these helpers. ChangesUTF-8 boundary handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant String_rindex
participant mrb_utf8_char_head
participant StringBuffer
String_rindex->>mrb_utf8_char_head: resolve the previous character boundary
mrb_utf8_char_head->>StringBuffer: inspect bytes and UTF-8 validity
StringBuffer-->>mrb_utf8_char_head: provide byte boundary
mrb_utf8_char_head-->>String_rindex: return the character head
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
include/mruby/internal.h (1)
198-209: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the non-empty input precondition for
mrb_utf8len.
mrb_utf8lenreadsstr[0]before any bounds test. The comment does not state thatstr < endis required.mrb_utf8_char_headdiffers, because it testsp >= endfirst. State this difference so new callers do not pass an empty range.📝 Proposed comment addition
/* What a run of bytes spells, which every build answers the same way, whether or not its strings index by character. mrb_utf8len returns the byte length of - the character at `str`, and 1 for anything that spells no character: a byte + the character at `str`, which must satisfy `str < end`, and 1 for anything + that spells no character: a byte that starts no sequence, a sequence `end` cuts short, one whose continuation bytes are not continuation bytes, and one that RFC 3629 forbids (an overlong encoding, a UTF-16 surrogate, a code point above U+10FFFF).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@include/mruby/internal.h` around lines 198 - 209, Update the comment for mrb_utf8len to explicitly require a non-empty range, with str < end, because it reads str[0] before checking bounds. Contrast this with mrb_utf8_char_head, which safely handles p >= end, so callers do not pass an empty range to mrb_utf8len.test/t/string.rb (1)
662-682: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding a truncated-sequence case.
The tests cover an orphan continuation byte, an overlong sequence, and a surrogate. They do not cover a lead byte whose sequence the string end cuts short. That path is the
len > e - pbranch ofmrb_utf8lenand it drivesmrb_utf8_char_headback-scan behavior at the end of the buffer.🧪 Proposed test addition
assert_equal 3, "\xED\xA0\x80".length assert_equal 2, "\xED\xA0\x80".rindex("\x80") + + # A lead byte the string end cuts short spells no character either. + assert_equal 2, "a\xE3\x81".length + assert_equal 1, "a\xE3\x81".rindex("\xE3") end if UTF8STRINGConfirm the expected
lengthvalue against the core implementation before you apply this diff.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/t/string.rb` around lines 662 - 682, Add a truncated UTF-8 sequence case to the String#rindex test, using a lead byte whose continuation sequence is cut off at the string end. Confirm the expected String#length from mrb_utf8len, then assert the length and relevant rindex behavior to exercise the mrb_utf8_char_head end-of-buffer back-scan path alongside the existing malformed-sequence cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@include/mruby/internal.h`:
- Around line 198-209: Update the comment for mrb_utf8len to explicitly require
a non-empty range, with str < end, because it reads str[0] before checking
bounds. Contrast this with mrb_utf8_char_head, which safely handles p >= end, so
callers do not pass an empty range to mrb_utf8len.
In `@test/t/string.rb`:
- Around line 662-682: Add a truncated UTF-8 sequence case to the String#rindex
test, using a lead byte whose continuation sequence is cut off at the string
end. Confirm the expected String#length from mrb_utf8len, then assert the length
and relevant rindex behavior to exercise the mrb_utf8_char_head end-of-buffer
back-scan path alongside the existing malformed-sequence cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ccc71475-5bcf-47af-b0f5-eb7df369c2b9
📒 Files selected for processing (6)
include/mruby/internal.hmrbgems/mruby-regexp/include/re_internal.hmrbgems/mruby-regexp/src/re_compile.cmrbgems/mruby-regexp/src/re_utf8.csrc/string.ctest/t/string.rb
59cf80c to
e33179b
Compare
|
@coderabbitai Both applied in e33179b, which is also rebased onto master now that #7107 has merged.
Every caller in the tree already holds to it: the walks test The truncated sequence. Added, though not with the expected values the diff proposed. # A lead byte the string end cuts short reaches none of the bytes that
# follow it, so those stand alone too.
assert_equal 3, "a\xE3\x81".length
assert_equal 1, "a\xE3\x81".rindex("\xE3")
assert_equal 2, "a\xE3\x81".rindex("\x81")It is worth having, since it is the case that pins Green after the change on both builds, 2248 tests on full-core and 2057 on the default gembox, and the first commit is green on its own. |
|
Tip For best results, initiate chat on the files or code changes.
The revised truncated-sequence expectations are correct. ✏️ Learnings added
🧠 Learnings usedIf you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
`MRB_UTF8_STRING` says that a string indexes by character. What a run of bytes spells is a question apart from that, and mruby-regexp asks it whatever the build does: it carries its own copy of both answers because core's were behind the guard. Move the two that are only about the bytes out from under it. Nothing else follows: `mrb_utf8_strlen()`, the character and byte conversions and the searches stay where they were, since those are the indexing. `str_char_head()` is now `mrb_utf8_char_head()` and declared in internal.h beside `mrb_utf8len()`, the function whose answer it is built on.
`mrb_re_utf8_charlen()` and `mrb_re_utf8_interior_p()` answer the same two questions as core's `mrb_utf8len()` and `mrb_utf8_char_head()`, byte for byte. They exist because core kept both behind `MRB_UTF8_STRING`, which the previous commit ended: the engine reads UTF-8 whatever a build's strings index by, and the default gembox already builds it that way, with mruby-regexp and without mruby-encoding. Call core's instead. `mrb_re_utf8_decode()` stays, since core offers no decoder, but takes its byte length from `mrb_utf8len()` too. The predicate keeps its own answer for a byte that starts a character. That is most of the positions the matcher tries, and each of them is now a branch rather than a call. It also stops reading `*s` at the end of the subject, where it used to lean on the terminating NUL to answer the same way. Equivalence was checked by brute force: 2,359,296 cases for the byte length (both first bytes exhaustively, by nine representative tails, by every available length) and 33,338,661 positions for the predicate (every buffer of up to five bytes over a 23-value alphabet covering each lead, continuation and discriminant class). No differences. The same run checks each head against a forward walk from the start of the buffer, which is what makes the three-byte lookback exact rather than an approximation of one.
e33179b to
56a55f5
Compare
2 participants
mruby-regexp carries its own answers to two questions core already answers:
how many bytes the character at a pointer takes, and where the character
covering a pointer starts.
mrb_re_utf8_charlen()and core'smrb_utf8len()agree byte for byte, and
mrb_re_utf8_interior_p()applies the same rule as thestr_char_head()that #7107 arrived at.They are separate because core keeps
mrb_utf8len()behindMRB_UTF8_STRING,and the regexp engine has no such guard: it reads UTF-8 whatever a build's
strings index by. That is not a hypothetical configuration. mruby-regexp is in
the stdlib gembox and mruby-encoding is not, so the stock default gembox
already builds the engine without
MRB_UTF8_STRING, decoding UTF-8 with itsown copy while core's sits compiled out.
What moves
MRB_UTF8_STRINGsays a string indexes by character. What a run of bytes spellsis a question apart from that, so the two functions that only answer it come out
from under the guard:
mrb_utf8len(), unchangedmrb_utf8_char_head(), which is string.c: stepString#rindexby the charactersString#lengthcounts #7107'sstr_char_head()renamed anddeclared in internal.h beside it
Nothing else follows.
mrb_utf8_strlen(), the character and byte conversionsand the searches stay behind the guard, because those are the indexing.
What the engine drops
mrb_re_utf8_charlen()is deleted;mrb_re_charlen()andemit_char_bytes()callmrb_utf8len()mrb_re_utf8_interior_p()becomesmrb_utf8_char_head(...) != smrb_re_utf8_decode()stays, since core offers no decoder, but takes itsbyte length from
mrb_utf8len()tooThe predicate keeps its own answer for a byte that starts a character, so most
of the positions the matcher tries stay a branch rather than a call. It also
stops reading
*sat the end of the subject, where it used to lean on theterminating NUL to answer the same way.
Equivalence
Checked by brute force rather than argued:
mrb_re_utf8_charlen()againstmrb_utf8len(): 2,359,296 cases,both first bytes exhaustively, by nine representative tail bytes, by every
available length from 1 to 4
to five bytes over a 23-value alphabet covering each lead, continuation and
discriminant class, at every position
No differences in either. The same run checks each head against a forward walk
from the start of the buffer, which is what makes the three-byte lookback exact
rather than an approximation of one: if a character covers
pand startsbackbytes earlier, every byte in between is a continuation byte, so the nearest
non-continuation byte going back is that character's lead, and that lead cannot
belong to an earlier character in turn.
Size
Deleting one copy costs less than exposing the other, in both directions:
MRB_UTF8_STRING)size(1)text ofbin/mruby, gcc-O2on x86-64.Base
Branched off #7107, which is still open, because
str_char_head()arrivesthere. The first two commits here are that PR and drop out once it merges.
Verified
rake teston a full-core build: 2248 tests, all greenrake teston the default gembox, which is the build where the engine readsUTF-8 without
MRB_UTF8_STRING: 2057 tests, all greenprek run --all-filespasses, except thatmarkdownlintcould not installlocally (npm engine mismatch); no Markdown is touched here
Summary by CodeRabbit
String#rindexbehavior with negative positions, multibyte characters, invalid UTF-8 bytes, and boundary limits.String#rindexbehavior and edge cases.