Skip to content

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