Fix matched_size when the stored string is shrunk - #217
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The changes are covered by regression tests and have no unresolved blocking issues.
Pull request overview
Updates matched_size across CRuby, JRuby, and TruffleRuby to respect shortened strings.
Changes:
- Clamp matched ranges to the current string length.
- Return
nilwhen the match begins beyond the string. - Add shared regression tests.
File summaries
| File | Summary |
|---|---|
test/strscan/test_stringscanner.rb |
Adds shrinking-string regression tests. |
lib/strscan/truffleruby.rb |
Corrects TruffleRuby matched-size handling. |
ext/strscan/strscan.c |
Corrects CRuby matched-size handling. |
ext/jruby/org/jruby/ext/strscan/RubyStringScanner.java |
Corrects JRuby matched-size handling. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
kou
left a comment
kou
left a comment
There was a problem hiding this comment.
Could you simplify the PR description?
|
Simplified — thanks, and sorry for the wall of text. |
matched_size returned the raw register width, so after the stored
string was shrunk under the scanner it disagreed with matched:
s = StringScanner.new(+"before 29 after")
s.skip_until(" ")
s.scan(/\d+/) #=> "29"
s.string.replace("before ")
s.matched #=> ""
s.matched_size #=> 2, should be 0
matched answers correctly because extract_range clamps to the current
length of the string. integer_at was given the same two lines in ruby#212,
charpos in 7b77f30, and bol? carries its own guard. matched_size was
missed in all three implementations.
Clamp it the same way, and return nil once the match start is past the
end of the string, which is what extract_range does.
7f947b3 to
d1b0afd
Compare
|
The truffleruby rows were red, and the cause turned out to be worth widening the PR slightly.
beg = Primitive.match_data_byte_begin(@last_match, 0)
return if beg > @string.bytesize
fin = [Primitive.match_data_byte_end(@last_match, 0), @string.bytesize].min
@string.byteslice(beg, fin - beg)All four backends now use the same shape as CRuby is green locally — 167 tests, 1114 assertions, 0 failures. One thing I could not verify locally, so I would rather say it than imply otherwise: |
kou
left a comment
kou
left a comment
There was a problem hiding this comment.
+1
We may want to change [], pre_match and so on in the TruffleRuby implementation for shrunk case as a separated tasks.
(ruby/strscan#217) `matched_size` returns the raw register width without clamping it to the current length of the stored string, so shrinking the string leaves it reporting the old match: ```ruby s = StringScanner.new("abcdef") s.scan(/abcdef/) s.string = "abc" s.matched_size # => 6, should be 3 s.matched # => "abc" (this one is already correct) ``` `extract_range` (`ext/strscan/strscan.c:168-169`) already clamps, which is why `matched` answers correctly. #212 added the same two lines to `integer_at`, `charpos` got them in `7b77f30`, and `bol?` carries its own guard — `matched_size` was missed. Fixed in all three backends (CRuby, JRuby, TruffleRuby) with a shared regression test. --------- ruby/strscan@0906399858 Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
matched_sizereturns the raw register width without clamping it to the current length of the stored string, so shrinking the string leaves it reporting the old match:extract_range(ext/strscan/strscan.c:168-169) already clamps, which is whymatchedanswers correctly. #212 added the same two lines tointeger_at,charposgot them in7b77f30, andbol?carries its own guard —matched_sizewas missed.Fixed in all three backends (CRuby, JRuby, TruffleRuby) with a shared regression test.