string.c: String#chop! takes a byte off a binary string - #7084
Merged
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)
📝 WalkthroughWalkthrough
ChangesBinary String Chop Handling
Estimated code review effort: 2 (Simple) | ~10 minutes 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 |
`mrb_str_chop_bang()` walks the string with `mrb_utf8len()` to find where the
last character starts. A string marked by `String#b` is read as bytes
everywhere else and ends in a byte rather than in a character, and the walk
never asked about the flag, so it took off the whole of a multi-byte sequence.
```ruby
"\u{1F600}".b.dup.chop! # F0 9F 98 80: was "", CRuby: "\xF0\x9F\x98"
"a\u{1F600}".b.dup.chop! # was "a", CRuby: "a\xF0\x9F\x98"
```
A string that held one character came back empty, which is the same answer
`chop!` gives for a one-character string read as UTF-8 and was reached here by
a rule that does not apply.
Read the flag and take the last byte. The `\r\n` pair is still taken together,
since that test runs on the byte before the end either way.
`String#chop` copies the string first and `str_replace()` does not carry
`MRB_STR_BINARY` to the copy, so it answers as before until the flag survives
a copy.
takumin
force-pushed
the
string-binary-chop
branch
from
August 10, 2026 16:31
6c9721d to
f981565
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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`:
- Around line 1870-1882: Update mrb_str_chop and its duplication path so the
copied receiver retains MRB_STR_BINARY before invoking the chop helper, ensuring
String#chop matches chop! for binary strings by removing one byte. Add a
regression test covering String#chop on a binary string under MRB_UTF8_STRING.
🪄 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: 3cb19dae-e035-459e-a591-b0932bbff0ab
📒 Files selected for processing (2)
mrbgems/mruby-encoding/test/string.rbsrc/string.c
This was referenced Aug 12, 2026
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
2 participants
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.
mrb_str_chop_bang()walks the string withmrb_utf8len()to find where thelast character starts. A string marked by
String#bis read as byteseverywhere else and ends in a byte rather than in a character, and the walk
never asked about the flag, so it took off the whole of a multi-byte sequence.
A string that held one character came back empty, which is the right answer
for a one-character string read as UTF-8 and was reached here by a rule that
does not apply to a string of bytes.
The change
mrb_str_chop_bang()takes the last byte whenRSTR_BINARY_P()says thestring is byte-indexed. The
\r\npair is still taken together, since thattest runs on the byte before the end either way.
Related, not required
String#chopcopies the string first andstr_replace()does not carryMRB_STR_BINARYto the copy, so it answers as before; #7080 makes the flagsurvive a copy and the copying form follows. #7081 fixes the length for the
same kind of string, and
String#inspecthas a walk of its own. Each is aseparate fix and none depends on another landing first.
Testing
rake testis green onfull-corewith gcc onx86_64-linux: 2215 asserts,no failures. The new test fails without the change:
The test also pins what must not move: a string read as UTF-8 still loses the
whole character, and
"a\r\n".bstill loses both bytes of the pair. Checkedagainst CRuby 4.0.6.
Summary by CodeRabbit
Bug Fixes
String#chop!for binary strings in UTF-8 builds. It now removes exactly the final byte without incorrectly interpreting multi-byte data as text.Tests