Skip to content

Share character/byte offset conversion between core and mruby-regexp - #7097

Merged
matz merged 2 commits into
mruby:masterfrom
takumin:string-share-char-byte-conversion
Aug 12, 2026
Merged

Share character/byte offset conversion between core and mruby-regexp#7097
matz merged 2 commits into
mruby:masterfrom
takumin:string-share-char-byte-conversion

Conversation

@takumin

@takumin takumin commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

src/string.c and mruby-regexp each carry their own character/byte offset
conversion: the core statics chars2bytes / bytes2chars, and the gem's
re_char_to_byte / re_byte_to_char with duplicated #ifdef MRB_UTF8_STRING
branches. This PR promotes the core pair to internal API and rebuilds the
regexp pair on top of it.

Core

chars2bytes and bytes2chars become mrb_str_char_to_byte(mrb, str, off, nchars) and mrb_str_byte_to_char(mrb, str, bi), declared in internal.h
outside the MRB_UTF8_STRING guard, following the same pattern as
mrb_utf8_to_buf: the symbols exist on every build, and a non-UTF-8 build
gets identity functions in place of the former identity macros, so callers
need no conditional compilation. The conversion logic itself is unchanged,
and the off parameter stays because String#[] resolves a range by
converting the start first and then measuring the length from that byte
offset, avoiding a rescan of the prefix. Call sites inside string.c are in
the same translation unit and still inline, so generated code there does not
change.

mruby-regexp

re_byte_to_char and re_char_to_byte keep only the policy the gem owns
(negative position normalization for Regexp#match, -1 for out-of-range
positions, unmatched capture offsets passing through negative) and delegate
the walking to the shared functions. Both private loops and their
#ifdef MRB_UTF8_STRING / #else halves 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 stray
continuation bytes, so the same match could answer differently depending on
whether the single-byte flag had been computed yet:

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#[]. A negative pos
argument 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 test with build_config/host-debug.rb (full-core, so mruby-encoding
    defines MRB_UTF8_STRING): mrbtest 2229 OK / 0 KO, bintest 116 OK.
  • rake test with the default config (byte strings, identity conversion):
    mrbtest 2030 OK / 0 KO, bintest 105 OK.
  • The new assertions in mruby-regexp/test/regexp_utf8.rb fail against the
    previous conversion (begin, end, String#[] agreement, and negative
    pos all answer differently there) and pass with this change on both
    builds.

Summary by CodeRabbit

  • Bug Fixes
    • Improved character and byte position handling for UTF-8 strings, including malformed input and out-of-range offsets.
    • Corrected regular-expression match positions for positive and negative offsets.
    • Ensured matching at the end of a string returns the expected empty match.
    • Improved consistency across string indexing, slicing, replacement, reversing, and splitting operations.
  • Tests
    • Added coverage for malformed UTF-8 position handling and end-of-string matches.

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.
@takumin
takumin requested a review from matz as a code owner August 12, 2026 03:42
@coderabbitai

coderabbitai Bot commented Aug 12, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The 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.

Changes

String character-byte offset conversion

Layer / File(s) Summary
Conversion API contract
include/mruby/internal.h, src/string.c
The change exposes UTF-8 character-to-byte and byte-to-character APIs. Non-UTF-8 builds use identity conversion.
String path integration
src/string.c
Indexing, substring extraction, replacement, reverse indexing, and splitting use the shared conversion APIs.
Regexp position handling
mrbgems/mruby-regexp/src/regexp.c, mrbgems/mruby-regexp/test/regexp_utf8.rb
Regexp offset conversion handles malformed UTF-8, oversized and negative offsets, invalid positions, and end-of-string matches. Tests cover these cases.

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
Loading

Possibly related PRs

Suggested labels: core, mrbgems

Suggested reviewers: matz, dearblue

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: sharing character and byte offset conversion APIs between mruby core and mruby-regexp.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Validate byte offsets and preserve byte indexing for binary strings.

  • Validate bi before the fast path and before p + bi. Invalid offsets can cause undefined pointer arithmetic, and fast paths return offsets past the end instead of -1.
  • Route RSTR_BINARY_P strings through mrb_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

📥 Commits

Reviewing files that changed from the base of the PR and between 9fc64e5 and 6248b58.

📒 Files selected for processing (4)
  • include/mruby/internal.h
  • mrbgems/mruby-regexp/src/regexp.c
  • mrbgems/mruby-regexp/test/regexp_utf8.rb
  • src/string.c

Comment thread src/string.c
@matz
matz merged commit db31680 into mruby:master Aug 12, 2026
21 checks passed
@takumin

takumin commented Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

On the outside diff comment about mrb_str_byte_to_char (lines 603-615):

The bounds check landed as #7098. The test now sits above both the fast path and
p + bi, so an offset outside the string returns -1 instead of forming an out of
bounds pointer, on UTF-8 and non-UTF-8 builds alike.

The second half of that comment does not apply. mrb_str_byte_to_char is not on
the String#rindex dispatch path, so there is nothing to route to
mrb_str_byterindex_m from here. A binary string in this function takes the
identity fast path, which is the correct answer for a subject indexed by byte.
The rindex dispatch itself is the inline comment on line 2503, handled in #7099.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants