Repository navigation
Conversation
Recursion to group0 can return to a caller that needs a terminal iterator to give back characters. Track whole-pattern calls separately and prohibit the OP_END possessification in that case, retaining the optimization for patterns with only local subroutine calls. Add matching regressions for (?R) and (?0), and a local-recursion control that retains the possessive terminal iterator. Fixes: PCRE2Project#1034 Assisted-by: OpenAI Codex Reviewed-by: GPT-6 Luna (two independent AI reviews) Signed-off-by: qeqx <284521809+anon5376@users.noreply.github.com>
|
Thanks! I'll review this, but it seems a very safe change, and it's important to fix the issue. |
There was a problem hiding this comment.
🟢 Approval recommended
The focused implementation correctly distinguishes whole-pattern recursion and includes representative regression coverage.
0 open findings
What changed in this PR
Fixes incorrect auto-possessification when whole-pattern recursion requires terminal iterator backtracking.
Changes:
- Tracks recursion into group 0 separately.
- Prevents possessifying affected terminal iterators.
- Adds recursion regression and subroutine control tests.
| File | Description |
|---|---|
src/pcre2_intmodedep.h |
Adds whole-pattern recursion state. |
src/pcre2_compile.c |
Detects and initializes that state. |
src/pcre2_auto_possess.c |
Preserves terminal iterator backtracking. |
testdata/testinput5 |
Adds regression and control cases. |
testdata/testoutput5 |
Records expected results and opcodes. |
ChangeLog |
Documents the fix. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
NWilson
left a comment
NWilson
left a comment
There was a problem hiding this comment.
OK, I like the change. I would like to request just a couple of things:
- Your test in testinput5 only exercises the JIT and interpreter. Ideally, I'd like this to go in the DFA regression tests too.
Add this to testinput6:
/(?:b(?R)a|a+)/
baa
baaa
-
The file testinput5 is for Unicode tests; however the tests you added don't seem to require it, so should go in testinput2.
-
Finally, something from GPT, not me. It observes that the tests you added don't exercise very many cases for auto-possessification. Can we add these to testinput2 as well:
# Confirm that non-terminal auto-possessification applies even with (?R)
/(?:b(?R)a|a+b)/B
# Confirm behaviour with character class possessification as well
/(?:b(?R)a|[ac]+)/B
baa
(?:b(?R)a|a+)matchesaainsidebaainstead of the fullbaa. Auto-possessification prevents the recursive call from giving back the finala.Track whole-pattern recursion separately and keep terminal iterators backtrackable. Ordinary subroutine optimization stays enabled.
Fixes #1034.
Tests: new
(?R)and(?0)cases fail before the fix across 8/16/32-bit interpreter and JIT runs. All four CTest suites pass after. Includes a control that retains subroutine auto-possessification.I’ve reviewed the code.