Skip to content

Add Closure::fromCallable(). #1906

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 4 commits into from
Jul 5, 2016
Merged

Add Closure::fromCallable(). #1906

merged 4 commits into from
Jul 5, 2016

Conversation

Danack
Copy link
Contributor

@Danack Danack commented May 15, 2016

Add the ability to create closures from callable as part of RFC: https://wiki.php.net/rfc/closurefromcallable



/* {{{ proto Closure Closure::fromCallable(callable callable)
Create a closure from a callabl using the current scope. */
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: callabl

memset(&call, 0, sizeof(zend_internal_function));

call.type = ZEND_INTERNAL_FUNCTION;
call.handler = zend_closure_call_magic;
Copy link
Member

Choose a reason for hiding this comment

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

Call through zend_call_function() is more expensive than through TRAMPOLINE.
Why it's not possible to reuse the same trampoline?
May be it makes sense to move trampoline function into zend_closure and free it together with closure object...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dmitry - I don't really understand trampolines. So long as this PR is acceptable and the tests all pass, is it okay for you to do any performance improvements you think are good, after this PR is accepted and closed?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah. Commit this. I'll take a look, if it's possible to optimise this using trampolines later.

@@ -0,0 +1,48 @@
--TEST--
Imagick::readImage test
Copy link
Member

@nikic nikic Jun 4, 2016

Choose a reason for hiding this comment

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

Nope :P

(And the SKIPIF)

@krakjoe
Copy link
Member

krakjoe commented Jun 8, 2016

Can someone tag this with RFC and accepted, it is destined for 7.1.

@php-pulls php-pulls merged commit fc92eee into php:master Jul 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants