Skip to content

RFC: Turn clone() into a function #18919

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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

TimWolla
Copy link
Member

This is necessary for `clone()` which does not currently accept a
parameter-less syntax, unless fully-qualified.
TimWolla added a commit to TimWolla/PHP-Parser that referenced this pull request Jun 23, 2025
nikic pushed a commit to nikic/PHP-Parser that referenced this pull request Jun 23, 2025
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 in general

$a[0]->b = 0;
try {
$a = clone 0;
$a[0]->b = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Useless line

array_push($a->b, $c);
try {
$a = clone(null);
array_push($a->b, $c);
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Useless line

RETURN_THROWS();
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This duplicates some logic from zend_vm_def.h. Can we consolidate? If not, we may at least link to the other function so that they are kept in-sync.

}
}

zend_object *cloned;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Combine declaration & init.

{
znode arg_node;

if (args->children != 1) {
Copy link
Member

Choose a reason for hiding this comment

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

Could be an ZEND_ASSERT for now.

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.

2 participants