Skip to content

test: seed the group a backward search miss has to clear - #7151

Merged
matz merged 1 commit into
mruby:masterfrom
takumin:rsearch-miss-clears-every-global
Aug 14, 2026
Merged

test: seed the group a backward search miss has to clear#7151
matz merged 1 commit into
mruby:masterfrom
takumin:rsearch-miss-clears-every-global

Conversation

@takumin

@takumin takumin commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Follow-up to #7149, on a review comment it left behind.

#7149 added a test that asks what a backward search leaves in the match globals, ending with the miss: a search that finds nothing clears all of them, not $~ alone. The block seeded one group and read six names back, and two of the seven it means to cover were not covered.

$~ is the one the comment names, and no assertion asked for it. $2 was nil before the call, because "zzz" =~ /(z)/ never filled it, so assert_nil $2 would have passed on a clear that missed it just as well as on one that did not. Seeding /(z)(z)/ is what makes the assertion stand for the clear.

"zzz" =~ /(z)(z)/
"abc".rindex(/x/)   #=> nil
$~                  #=> nil
$2                  #=> nil, and it was "z" before the call

Every answer is CRuby's.

Verification

Dropping the clear from __regexp_rsearch, which is the mistake the block is there to catch, turns 7 of its assertions red where it turned 5 before, $~ and $2 among them.

Full suite green on build_config/ci/gcc-clang.rb: full-core with MRB_GC_STRESS (2286), bintest (2286), the C++ ABI (2285), and the default gembox (2070). 0 failures, 0 crashes, no new compiler warnings.

Summary by CodeRabbit

  • Bug Fixes
    • Improved regular-expression backward-search behavior by ensuring all match results are cleared after an unsuccessful search.
    • Added coverage for multiple capture groups to prevent stale match data from being retained.

The test that asks whether a failing backward search clears every match
global, and not `$~` alone, seeded one group and then read six names back.
Two of the seven it means to cover were not covered: `$~` itself, which the
comment above the block names and no assertion asked for, and `$2`, which was
nil before the call because no seed ever filled it, so a clear that missed it
would have gone unseen.

Seed two groups and assert both. All seven names now stand for something the
call cleared: dropping the clear from `__regexp_rsearch` turns 7 of them red
where it turned 5 before.
@takumin
takumin requested a review from matz as a code owner August 14, 2026 05:41
@coderabbitai

coderabbitai Bot commented Aug 14, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 579c5f54-f24a-40e4-bebb-f81c169a868f

📥 Commits

Reviewing files that changed from the base of the PR and between 0ebee4a and e2556de.

📒 Files selected for processing (1)
  • mrbgems/mruby-regexp/test/string_index.rb

📝 Walkthrough

Walkthrough

The failed rindex test now seeds two capture groups and verifies that all match globals, including $2, are cleared when the search fails.

Changes

rindex Match Global Clearing

Layer / File(s) Summary
Failed rindex assertions
mrbgems/mruby-regexp/test/string_index.rb
The test seeds two capture groups and verifies that $~, $&, pre-match, post-match, $1, and $2 are cleared after a failed rindex.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Merge Risk: ⚪ Minimal · up to e2556

This change only strengthens regression coverage for backward-search misses, with no production behavior change; no actionable merge-blocking risk remains after normal checks and review.

Possibly related PRs

  • mruby/mruby#7025: Extends tests for clearing capture globals after failed rindex.
  • mruby/mruby#7057: Changes capture-group and numbered match-global assignment.
  • mruby/mruby#7149: Extends regression coverage for match-global clearing in string_index.rb.

Suggested reviewers: matz

🚥 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 describes the regression test change for seeding a capture group before a backward search miss clears match state.
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.

@matz
matz merged commit 84a4186 into mruby:master Aug 14, 2026
20 of 21 checks passed
@takumin
takumin deleted the rsearch-miss-clears-every-global branch August 14, 2026 05:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants