Skip to content
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

Fix instable array during in-place modification in uksort #13285

Closed
wants to merge 1 commit into from

Conversation

iluuu1994
Copy link
Member

The array isn't just observable if the array has RCn, but also if it is inside a reference that is RCn. By-ref parameters are always RCn and as such always observable.

Fixes GH-13279

The array isn't just observable if the array has RCn, but also if it is inside a
reference that is RCn. By-ref parameters are always RCn and as such always
observable.

Fixes phpGH-13279
@iluuu1994
Copy link
Member Author

By-ref parameters are always RCn and as such always observable.

Actually, this is wrong. CVs pass the reference without increasing the refcount. So there might be some benefit to this after all. I'll check this after lunch.

@iluuu1994 iluuu1994 marked this pull request as draft January 31, 2024 12:47
@nielsdos
Copy link
Member

Thank you Ilija!
I'm fine with this as-is to be honest.
How commonly, after correctly fixing this, would the optimization be used in real-world code? I expect very few times when adding additional restrictions. So therefore, I prefer simpler code over potentially hard-to-understand code when the benefit is negligible. :-)

@iluuu1994
Copy link
Member Author

I agree. My last comment was also incorrect. I was thinking of CV opcode args with their refcount not being increased, rather than function arguments. So checking for RC1 will be useless.

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.

Let's go with this

@@ -0,0 +1,18 @@
--TEST--
Copy link
Member

Choose a reason for hiding this comment

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

I guess this should go to ext/standard/tests/array

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. TBH the separation of these tests is often quite arbitrary. I usually just throw stuff in Zend.

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.

None yet

3 participants