Skip to content

Fix bug #73948: Trailing NULL padding for UNMATCHED_AS_NULL #3964

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

nikic
Copy link
Member

@nikic nikic commented Mar 19, 2019

This is a fix for https://bugs.php.net/bug.php?id=73948, targeting 7.4 for BC concerns.

There's two changes here affecting UNMATCHED_AS_NULL mode:

  • Make sure we always have the same number of results. If some of the unmatched subpatterns are at the end, we insert appropriate padding NULLs.
  • When using offset capture, instead of creating an array [null, -1], insert just null.

I'm not so sure about the second bit. I think this is how this should work, but possibly we need to stick with the current behavior for BC.

/cc @cmb69

@nikic nikic force-pushed the unmatched_as_null branch from 62288d7 to 7600e13 Compare March 19, 2019 12:24
@nikic
Copy link
Member Author

nikic commented Mar 19, 2019

After thinking about this a bit more, I'm dropping the second point. There's value in always having the same format... Now this change only includes padding with null or [null, -1]. I've implemented an optimization in 4fe3d10 that avoids creating a new array for each [null, -1] entry, so doing this shouldn't be much more expensive than using just null.

@petk petk added the Bug label Mar 19, 2019
@nikic
Copy link
Member Author

nikic commented Mar 21, 2019

Merged as 6311581. I think in this form the BC break here should be very limited... let's see if anyone complains :)

@nikic nikic closed this Mar 21, 2019
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