Closure::call()#775
Conversation
Conflicts: Zend/zend_closures.c Zend/zend_closures.h Zend/zend_execute_API.c Zend/zend_vm_def.h
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.
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.. |
👍 |
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