Skip to content

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

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

9 participants