Closure::call() #775
Closure::call() #775
Conversation
Conflicts: Zend/zend_closures.c Zend/zend_closures.h Zend/zend_execute_API.c Zend/zend_vm_def.h
| @@ -32,14 +32,16 @@ $cas->bindTo($a, 'A'); | |||
|
|
|||
| ?> | |||
| --EXPECTF-- | |||
| Notice: Array to string conversion in %s on line %d | |||
| Notice: Array to string conversion in %s on line 24 | |||
There was a problem hiding this comment.
why you change %d here? by accident?
There was a problem hiding this comment.
Initially yes, but I don't think you should use %d with line numbers as they're not going to change.
| zend_function my_function; | ||
|
|
||
| if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "o*", &newthis, &my_params, &my_param_count) == FAILURE) { | ||
| RETURN_NULL(); |
There was a problem hiding this comment.
simply return; here is enough(it's a tradition :)
|
@laruence |
|
Also, |
|
@TazeTSchnitzel Because of the this ;-) |
|
@datibbaw zpp is wrong though, you're almost always calling it with |
|
@TazeTSchnitzel in that case, I would like make fci.param_count as int instead of include a php/stdint.h into Zend... |
|
or , maybe use zend_long.h here. (it depends on stdint.h now, I will definitely ask welting to re-think about it) |
|
anyway, please, use int for now... and do not include "dependence out of Zend" |
|
zend.h already includes the stdint header. You can use uint32_t here without any additional includes. |
|
@nikic Shouldn't I explicitly include what I use, rather than relying on zend.h happening to include a billion headers? |
|
zend_closure.c already includes zend.h. Source files should pretty much always include it. |
|
@nikic That's not my point. Surely I should still include it explicitly rather than relying on zend.h happening to include it? |
|
No, you should not include it explicitly. Only include the files that are not present in zend.h. |
|
@nikic Ah, fair enough. |
|
great, then problem gone.. |
👍 |
| @@ -9,6 +9,7 @@ PHP NEWS | |||
|
|
|||
| - Core: | |||
| . Added PHP_INT_MIN constant. (Andrea) | |||
| . Added Closure::apply() method. (Andrea) | |||
There was a problem hiding this comment.
The code says Closure:call(). Following JavaScript it should be called apply() and a varargs version should be calles call().
There was a problem hiding this comment.
No, it's called call for consistency with JS. JS's call isn't varargs. JS's apply is.
There was a problem hiding this comment.
Ah, misunderstood the implementation. Still, the NEWS file needs fixing. Do you plan on adding apply as well?
There was a problem hiding this comment.
The NEWS file was already fixed. And there's no need for apply, given we have splats.
The pull request of the RFC: https://wiki.php.net/rfc/closure_apply