Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Dedicated syntax for variadic parameters #421

Closed
wants to merge 1 commit into from

8 participants

@nikic
Owner

Implementation for https://wiki.php.net/rfc/variadics RFC.

@clemherreman

RFC can be found here: https://wiki.php.net/rfc/variadics

I genuinely find this a good thing, especially for the type hinting. Being able to explicitely indicate that a function is variadic via its signature is way better than looking at documentation or directly by looking at its code.

So a big +1 :)

@failpunk

Is the syntax space sensitive? Same examples have a space after the ellipsis and some do not.

public function query($query, ...$params) 

public function prepare($query, &... $params)
@nikic
Owner

@failpunk No, whitespace doesn't matter (as with all of PHP).

Zend/tests/errmsg_015.phpt
@@ -11,4 +11,4 @@ class test {
echo "Done\n";
?>
--EXPECTF--
-Fatal error: Method test::__clone() cannot accept any arguments in %s on line %d
+Fatal error: Method test::__clone() cannot take arguments in %s on line %d
@smalyshev Owner

Why this change I wonder?

@nikic Owner
nikic added a note

I refactored some of the magic method implementation checks (and changed the error message doing that), but that turned out to be unnecessary in the end. I now removed the changes to avoid cluttering the diff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Zend/zend_API.c
((56 lines not shown))
- if (fptr->common.num_args != 2) {
- zend_error(error_type, "Method %s::%s() must take exactly 2 arguments", ce->name, ZEND_CALL_FUNC_NAME);
- } else if (ARG_SHOULD_BE_SENT_BY_REF(fptr, 1) || ARG_SHOULD_BE_SENT_BY_REF(fptr, 2)) {
- zend_error(error_type, "Method %s::%s() cannot take arguments by reference", ce->name, ZEND_CALL_FUNC_NAME);
- }
- } else if (name_len == sizeof(ZEND_CALLSTATIC_FUNC_NAME) - 1 &&
- !memcmp(lcname, ZEND_CALLSTATIC_FUNC_NAME, sizeof(ZEND_CALLSTATIC_FUNC_NAME)-1)
- ) {
- if (fptr->common.num_args != 2) {
- zend_error(error_type, "Method %s::%s() must take exactly 2 arguments", ce->name, ZEND_CALLSTATIC_FUNC_NAME);
- } else if (ARG_SHOULD_BE_SENT_BY_REF(fptr, 1) || ARG_SHOULD_BE_SENT_BY_REF(fptr, 2)) {
- zend_error(error_type, "Method %s::%s() cannot take arguments by reference", ce->name, ZEND_CALLSTATIC_FUNC_NAME);
+ for (i = 0; i < method_info_count; ++i) {
+ if (name_len == method_info[i].name_len && !memcmp(lcname, method_info[i].name, method_info[i].name_len + 1)) {
+ int arg;
+ if (fptr->common.num_args != method_info[i].num_args) {
@smalyshev Owner

I wonder what if you define __call(...$params) - I think it should work. Would it work? Same with __destruct(...$params) - though I admit it is a weird case, but technically it should work too.

@nikic Owner
nikic added a note

Hm, I'm not sure about that. After all we also don't allow additional optional parameters for magic methods. So I don't think we should be allowing variadics either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Zend/zend_vm_def.h
@@ -3281,6 +3281,32 @@
ZEND_VM_NEXT_OPCODE();
}
+ZEND_VM_HANDLER(164, ZEND_RECV_VARIADIC, ANY, ANY)
+{
+ USE_OPLINE
+ zend_uint arg_num = opline->op1.num;
+ zend_uint arg_count = zend_vm_stack_get_args_count(TSRMLS_C);
+ zval **var_ptr, *params;
+
+ SAVE_OPLINE();
+
+ var_ptr = _get_zval_ptr_ptr_cv_BP_VAR_W(execute_data, opline->result.var TSRMLS_CC);
+ Z_DELREF_PP(var_ptr);
+ MAKE_STD_ZVAL(params);
+ array_init_size(params, arg_count - arg_num + 1);
@smalyshev Owner

Should there be a check for what happens if arg_count is way smaller than arg_num? I.e. if I call foo($a = 1, $b = 1, $c = 1, ...$params) with foo(), wouldn't arg_count be 0 and arg_num 4? then you'd allocate array of size -2.

@nikic Owner
nikic added a note

Yes, you're right. The current code doesn't account for optional parameters before ...$params.

@nikic Owner
nikic added a note

This is fixed now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@olekukonko

Its good to see PHP is about to support variadic functions just like in go but slightly different. I would like want to know if this code below return expected output :

function  A($x,$y,$z) {

    printf("%d,%d,%d",$x,$y,$z);
}

function B(...$vars) {
    A(...$vars);
}

function C($no, ...$vars) {
    foreach ($vars as &$var) {
        $var = $var * $no ;
    }
    A(...$vars);
}

B(1,2,3); // Expect  1,2,3  
C(2,1,2,3); // Expect  2,4,8  

Would this work this way or how do you achieve this with current implementation. Secondly can arrays be simple used. eg

A(...[1,10,100]); // Expect 1,10,100

//OR

$array = array_reverse(range(1,100))
A(...$array) // Expect 100,99,98 

I really this not see clarification of this usage case in the RFC. Can you please clarify ?

@smalyshev
Owner

@olekukonko you can always use call_user_func_array() for that. For the last example, I don't even see why would you bother to write A(...[1,10]) if you can just write A(1,10).

@olekukonko

@smalyshev that was just an example .... there is so many use cases for A(...$array)

@smalyshev
Owner

@olekukonko All of which, as far as I can see, are covered by call_user_func_array().

@slavcodev

May be use function fn(&$var, ...&$params) instead of function fn(&$var, &...$params)
It is more consistent.

@php-pulls
Collaborator

Comment on behalf of nikic at php.net:

Has been merged.

@php-pulls php-pulls closed this
@kaplanlior

For future reference, commit SHA1 is 0d7a638

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Sep 23, 2013
  1. @nikic
Something went wrong with that request. Please try again.