Skip to content

Propagate UTF-8 flag during Rope operations #10915

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

Merged
merged 2 commits into from
Mar 26, 2023
Merged

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Mar 23, 2023

Forgot to handle this concatenation case.

Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The propagation code for the VM looks good, but you also might need to update the JIT, in particular I think you need to update zend_jit_rope_end() in zend_jit_helpers.c.

@Girgias
Copy link
Member Author

Girgias commented Mar 24, 2023

The propagation code for the VM looks good, but you also might need to update the JIT, in particular I think you need to update zend_jit_rope_end() in zend_jit_helpers.c.

Ah yes, that looks like it needs to!

Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@iluuu1994
Copy link
Member

@Girgias I guess it makes sense to wait for Dmitry's response to see whether the VM overhead is ok.

@Girgias
Copy link
Member Author

Girgias commented Mar 26, 2023

@Girgias I guess it makes sense to wait for Dmitry's response to see whether the VM overhead is ok.

This is kinda a follow-up on #10463 and #10490, so the VM overhead has already been added. But in any case propagating this flags allows to not need to check certain strings are UTF-8 while using ext/mbstring, and probably could find more use cases elsewhere.

@Girgias Girgias merged commit d7c351e into php:master Mar 26, 2023
@Girgias Girgias deleted the rope-utf8 branch March 26, 2023 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants