Skip to content
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

Add Closure::fromCallable(). #1906

Merged
merged 4 commits into from Jul 5, 2016

Conversation

9 participants
@Danack
Copy link
Contributor

commented May 15, 2016

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

Add Closure::fromCallable().
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. */

This comment has been minimized.

Copy link
@pp3345

pp3345 May 15, 2016

Contributor

typo: callabl

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

call.type = ZEND_INTERNAL_FUNCTION;
call.handler = zend_closure_call_magic;

This comment has been minimized.

Copy link
@dstogov

dstogov May 16, 2016

Member

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...

This comment has been minimized.

Copy link
@Danack

Danack Jun 1, 2016

Author Contributor

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?

This comment has been minimized.

Copy link
@dstogov

dstogov Jun 2, 2016

Member

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

ZEND_BEGIN_ARG_INFO_EX(arginfo_closure, 0, 0, 1)
ZEND_ARG_INFO(0, callable)
ZEND_END_ARG_INFO()

This comment has been minimized.

Copy link
@nikic

nikic May 16, 2016

Member

What's this for? (And really, all the changes in this file).

This comment has been minimized.

Copy link
@Bilge

Bilge May 16, 2016

More changes = more enterprise.

This comment has been minimized.

Copy link
@Danack

Danack May 16, 2016

Author Contributor

@nikic I am bad with computers. I'll clean it up.

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

This comment has been minimized.

Copy link
@nikic

nikic Jun 4, 2016

Member

Nope :P

(And the SKIPIF)

}

if (Z_TYPE_P(callable) == IS_OBJECT && instanceof_function(Z_OBJCE_P(callable), zend_ce_closure)) {
// It's already a closure

This comment has been minimized.

Copy link
@nikic

nikic Jun 4, 2016

Member

For C89 compliance, should use /**/ style comments.

}

mptr = fcc.function_handler;
if (mptr == NULL) {

This comment has been minimized.

Copy link
@nikic

nikic Jun 4, 2016

Member

Can this happen?

}

ZVAL_OBJ(&instance, fcc.object);
zend_create_closure(return_value, mptr, mptr->common.scope, fcc.object ? fcc.object->ce : NULL, fcc.object ? &instance : NULL);

This comment has been minimized.

Copy link
@nikic

nikic Jun 4, 2016

Member

This should use zend_create_fake_closure, otherwise there will be much sadness.

This comment has been minimized.

Copy link
@nikic

nikic Jun 4, 2016

Member

I'm not sure that setting called_scope to NULL for static calls is what you want to do. Please check how something like this behaves:

class Foo {
    const BAR = 1;
    public static function method() {
        return static::BAR;
    }
}
var_dump(Closure::fromCallable(['Foo', 'method'])());

This comment has been minimized.

Copy link
@bwoebi

bwoebi Jul 6, 2016

Contributor

This case works as expected

This comment has been minimized.

Copy link
@nikic

nikic Jul 6, 2016

Member

Yes, because I already fixed it.

call.scope = mptr->common.scope;

zend_free_trampoline(mptr);
mptr = (zend_function *) &call;

This comment has been minimized.

Copy link
@nikic

nikic Jun 4, 2016

Member

This retains a reference to a stack value outside the block it was declared in.

call.type = ZEND_INTERNAL_FUNCTION;
call.handler = zend_closure_call_magic;
call.function_name = mptr->common.function_name;
call.arg_info = (zend_internal_arg_info *) mptr->common.prototype;

This comment has been minimized.

Copy link
@nikic

nikic Jun 4, 2016

Member

This doesn't look like the right member?


fcc.initialized = 1;
fcc.function_handler = (zend_function *) EX(func)->common.arg_info;
fci.params = (zval*) emalloc(sizeof(zval) * 2);

This comment has been minimized.

Copy link
@nikic

nikic Jun 4, 2016

Member

Better use stack allocation instead.

zval_ptr_dtor(&fci.params[0]);
zval_ptr_dtor(&fci.params[1]);
efree(fci.params);
}

This comment has been minimized.

Copy link
@nikic

nikic Jun 4, 2016

Member

Please add a test for a Closure::fromCallable([$closure, '__invoke'])(1, 2, 3). I suspect it will not be handled correctly...

This comment has been minimized.

Copy link
@nikic

nikic Jun 4, 2016

Member

Also generally, whatever issue this code is solving, does the same issue exist for ReflectionMethod::getClosure() and if not, how did they solve it?

@krakjoe

This comment has been minimized.

Copy link
Member

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.