Skip to content

Add Closure::fromCallable().#1906

Merged
php-pulls merged 4 commits intophp:masterfrom
Danack:closurefromcallable
Jul 5, 2016
Merged

Add Closure::fromCallable().#1906
php-pulls merged 4 commits intophp:masterfrom
Danack:closurefromcallable

Conversation

@Danack
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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