Skip to content

RFC: Clone with v2 #18747

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Jul 17, 2025
Merged

RFC: Clone with v2 #18747

merged 9 commits into from
Jul 17, 2025
+701 −10

Conversation

TimWolla
Copy link
Member

@TimWolla TimWolla commented Jun 3, 2025

RFC: https://wiki.php.net/rfc/clone_with_v2

see TimWolla#6 for a preliminary review.

@TimWolla TimWolla added the RFC label Jun 3, 2025
@TimWolla TimWolla mentioned this pull request Jun 3, 2025
Merged
@TimWolla TimWolla requested a review from dstogov as a code owner July 7, 2025 10:27
@TimWolla TimWolla requested review from iluuu1994 and nielsdos July 7, 2025 10:28
iluuu1994
iluuu1994 reviewed Jul 7, 2025
View reviewed changes
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.

Note that I did not (yet) carefully look at all tests.

@TimWolla TimWolla force-pushed the clone-with branch 2 times, most recently from 622bf8a to b5644ee Compare July 9, 2025 10:31
nielsdos
nielsdos requested changes Jul 15, 2025
View reviewed changes
TimWolla added a commit to TimWolla/php-src that referenced this pull request Jul 16, 2025
@TimWolla
zend_objects: Remove unnecessary refcounting when calling __clone()
2e7332b 
Found as part of the clone-with review in php#18747.
@TimWolla TimWolla mentioned this pull request Jul 16, 2025
Merged
TimWolla added a commit that referenced this pull request Jul 16, 2025
@TimWolla
zend_objects: Remove unnecessary refcounting when calling __clone() (…
2e0c011 
…#19145)

Found as part of the clone-with review in #18747.
@TimWolla TimWolla requested review from iluuu1994 and nielsdos July 17, 2025 07:18
iluuu1994
iluuu1994 approved these changes Jul 17, 2025
View reviewed changes
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, thanks Tim 🙂

nielsdos
nielsdos reviewed Jul 17, 2025
View reviewed changes
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.

I like how it turned out to be quite simple. One remark.

@TimWolla
Copy link
Member Author

I like how it turned out to be quite simple.

Indeed. It's a net addition of 70 lines (excluding tests):

 NEWS                                  |   3 +++
 UPGRADING                             |   3 ++-
 Zend/zend_builtin_functions.c         |  16 ++++++++++++++--
 Zend/zend_builtin_functions.stub.php  |   2 +-
 Zend/zend_builtin_functions_arginfo.h | Bin 15788 -> 15865 bytes
 Zend/zend_iterators.c                 |   1 +
 Zend/zend_object_handlers.c           |   1 +
 Zend/zend_object_handlers.h           |   2 ++
 Zend/zend_objects.c                   |  46 ++++++++++++++++++++++++++++++++++++++++++++++
 Zend/zend_objects.h                   |   1 +
 Zend/zend_vm_def.h                    |   3 ++-
 Zend/zend_vm_execute.h                | Bin 2280253 -> 2280621 bytes
 ext/com_dotnet/com_handlers.c         |   1 +
 ext/com_dotnet/com_saproxy.c          |   1 +
 14 files changed, 75 insertions(+), 5 deletions(-)

Thanks for the reviews. I've now did the final rebase including the latest remarks and added NEWS / UPGRADING. Will merge when CI is green.

@TimWolla TimWolla merged commit 7f4076b into php:master Jul 17, 2025
9 checks passed
@TimWolla TimWolla deleted the clone-with branch July 17, 2025 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iluuu1994 iluuu1994 iluuu1994 approved these changes

@nielsdos nielsdos nielsdos approved these changes

@kocsismate kocsismate Awaiting requested review from kocsismate kocsismate is a code owner

@dstogov dstogov Awaiting requested review from dstogov dstogov is a code owner

Assignees
No one assigned
Labels
ABI break Category: Engine Extension: com_dotnet RFC
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@TimWolla @iluuu1994 @nielsdos