Share character/byte offset conversion between core and mruby-regexp - #7097
Conversation
Rename the static `chars2bytes` and `bytes2chars` helpers to `mrb_str_char_to_byte` and `mrb_str_byte_to_char`, and declare them in internal.h outside the `MRB_UTF8_STRING` guard, so gems that need to translate between character indexes and byte offsets can share the core implementation instead of carrying their own. This follows the pattern of `mrb_utf8_to_buf`: the symbols exist on every build, and a build without `MRB_UTF8_STRING` gets identity functions in place of the former identity macros. Call sites inside string.c are in the same translation unit, so the compiler still inlines them there. The `off` parameter of the former `chars2bytes` is kept: `String#[]` converts a range by resolving the start first and then measuring the length from that byte offset, which avoids rescanning the prefix. Callers converting from the start of the string pass 0.
`re_byte_to_char` and `re_char_to_byte` carried private conversion loops that counted characters by skipping 10xxxxxx continuation bytes. Replace their bodies with calls to `mrb_str_byte_to_char` and `mrb_str_char_to_byte`, leaving only the policy this gem owns: negative position normalization for `Regexp#match`, -1 for out-of-range positions, and unmatched capture offsets passing through negative. Since the shared functions exist on every build, the gem's `#ifdef MRB_UTF8_STRING` copies of both functions go away too. Sharing the core walk also fixes an inconsistency on malformed UTF-8. The private loops counted lead bytes only, while core indexing counts every invalid byte as one character via `mrb_utf8len`, so the same match could answer differently depending on whether the single-byte flag had been computed yet: ```ruby s = "a\x80b" /b/.match(s).begin(0) # => 1, but s[1] is "\x80" s.length # => 3, marks the string single-byte /b/.match(s).begin(0) # => 2 ``` With the shared conversion both matches report 2, the position where `s[2]` is "b", agreeing with `String#length` and `String#[]`. An engine offset that lands inside a multibyte character, reachable only on malformed input, is backed up to the start of the containing character. The regression test pins the answer down before and after the length is computed.
📝 WalkthroughWalkthroughThe change exposes shared string character/byte conversion APIs, updates string operations to use them, and replaces manual regexp offset conversion. Tests cover malformed UTF-8 positions and matching at the end of a string. ChangesString character-byte offset conversion
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RegexpMatch
participant StringConversionAPI
participant UTF8String
RegexpMatch->>StringConversionAPI: Convert match byte and character offsets
StringConversionAPI->>UTF8String: Validate UTF-8 boundary
UTF8String-->>StringConversionAPI: Return converted position
StringConversionAPI-->>RegexpMatch: Return converted offset or -1
Possibly related PRs
Suggested labels: 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/string.c (1)
603-615: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate byte offsets and preserve byte indexing for binary strings.
- Validate
bibefore the fast path and beforep + bi. Invalid offsets can cause undefined pointer arithmetic, and fast paths return offsets past the end instead of-1.- Route
RSTR_BINARY_Pstrings throughmrb_str_byterindex_m. Otherwise, binary strings without the single-byte flag use UTF-8 backtracking for negative offsets and can return incorrect results.🤖 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 `@src/string.c` around lines 603 - 615, Update mrb_str_byte_to_char to validate bi before any fast-path return or p + bi pointer arithmetic, returning -1 for offsets outside the string bounds. Keep single-byte strings returning valid byte offsets unchanged, but route RSTR_BINARY_P strings through mrb_str_byterindex_m instead of the UTF-8 backtracking logic.
🤖 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.
Inline comments:
In `@src/string.c`:
- Line 2503: Update the early byte-index dispatch around mrb_str_char_to_byte so
binary strings identified by RSTR_BINARY_P are routed through byte-based rindex
handling, including negative positions, instead of the char_backtrack multibyte
path. Preserve the existing behavior for single-byte strings and normal
character-indexed strings.
---
Outside diff comments:
In `@src/string.c`:
- Around line 603-615: Update mrb_str_byte_to_char to validate bi before any
fast-path return or p + bi pointer arithmetic, returning -1 for offsets outside
the string bounds. Keep single-byte strings returning valid byte offsets
unchanged, but route RSTR_BINARY_P strings through mrb_str_byterindex_m instead
of the UTF-8 backtracking logic.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 613659ff-d9c7-4d79-a243-d13411ba6e7b
📒 Files selected for processing (4)
include/mruby/internal.hmrbgems/mruby-regexp/src/regexp.cmrbgems/mruby-regexp/test/regexp_utf8.rbsrc/string.c
|
On the outside diff comment about The bounds check landed as #7098. The test now sits above both the fast path and The second half of that comment does not apply. |
src/string.cand mruby-regexp each carry their own character/byte offsetconversion: the core statics
chars2bytes/bytes2chars, and the gem'sre_char_to_byte/re_byte_to_charwith duplicated#ifdef MRB_UTF8_STRINGbranches. This PR promotes the core pair to internal API and rebuilds the
regexp pair on top of it.
Core
chars2bytesandbytes2charsbecomemrb_str_char_to_byte(mrb, str, off, nchars)andmrb_str_byte_to_char(mrb, str, bi), declared ininternal.houtside the
MRB_UTF8_STRINGguard, following the same pattern asmrb_utf8_to_buf: the symbols exist on every build, and a non-UTF-8 buildgets identity functions in place of the former identity macros, so callers
need no conditional compilation. The conversion logic itself is unchanged,
and the
offparameter stays becauseString#[]resolves a range byconverting the start first and then measuring the length from that byte
offset, avoiding a rescan of the prefix. Call sites inside
string.care inthe same translation unit and still inline, so generated code there does not
change.
mruby-regexp
re_byte_to_charandre_char_to_bytekeep only the policy the gem owns(negative position normalization for
Regexp#match, -1 for out-of-rangepositions, unmatched capture offsets passing through negative) and delegate
the walking to the shared functions. Both private loops and their
#ifdef MRB_UTF8_STRING/#elsehalves disappear.Behavior fix on malformed UTF-8
The private regexp loops counted characters as "every byte that is not a
10xxxxxx continuation byte", while core indexing counts every invalid byte as
one character via
mrb_utf8len. The two disagree on strings with straycontinuation bytes, so the same match could answer differently depending on
whether the single-byte flag had been computed yet:
With the shared conversion both matches report 2, the position where
s[2]is
"b", agreeing withString#lengthandString#[]. A negativeposargument counts back from the end with the same rules, so
/a/.match(s, -3)now finds
"a"where it used to see a two character string and answer nil.Well-formed UTF-8 is unaffected. As a safety net, an engine offset that lands
inside a multibyte character, reachable only on malformed input, is backed up
to the start of the containing character. A regression test pins the
positions down before and after the length is computed, and was confirmed to
fail against the previous implementation.
Testing
rake testwithbuild_config/host-debug.rb(full-core, so mruby-encodingdefines
MRB_UTF8_STRING): mrbtest 2229 OK / 0 KO, bintest 116 OK.rake testwith the default config (byte strings, identity conversion):mrbtest 2030 OK / 0 KO, bintest 105 OK.
mruby-regexp/test/regexp_utf8.rbfail against theprevious conversion (
begin,end,String#[]agreement, and negativeposall answer differently there) and pass with this change on bothbuilds.
Summary by CodeRabbit