Hi Dmitry,

First of all: Your patch does really simplify things internally quite a
bit - I like it. I have a few issues though:

>  The patch shouldn't affect opcode caches and other extensions as it
>  doesn't change any structures.

I don't see a problem in changing structures for either extensions nor
opcode caches - as long as only entries are added. Binary compability
with PHP 5.2 is not provided anyway (by neither 5.3 nor 6) and source
compability is not affected if the old members are not touched or their
semantics change.

>  It uses the op_array->static_variables for lexical variables.

That's a point I don't like. Although you use IS_CONSTANT to cleverly
mask lexical variables, I really think a separate hash table would be a
far better idea, especially for code maintainability.

>  The patch also fixes several small issues and adds some missing
>  functionality which didn't allow preg_replace_callback() (and may be
>  others) to work with lambda functions.

Oh yes, I somehow missed that, thanks!

>  Please review.

I (personally) have some smaller issues with the patch and one big
issue:

Smaller issues:

  * A separate hash table for the lexical variables would be much cleaner
    in my eyes.

  * The segfault that occurs with my patch still occurs with yours (see
    below for an example)

But the one big issue is the syntax: ($foo | $bar) is just extremely
painful in my eyes. I wouldn't want to use it - and it would be quite
confusing (which side are the normal parameters, which side are the
lexical vars?). I do see your point that the 'lexical' keyword inside
the function body to actually have an effect on the function semantics
is not optimal and that the list of lexical variables is probably better
placed in the function definition. I therefore propose the following syntax:

function (parameters) { }                   // no closure, simply lambda
function (parameters) KEYWORD (lexical) { } // closure with lexical vars

KEYWORD could be for example 'use'. That probably describes best what
the function does: Use/import those variables from the current scope.
Example:

return function ($x) use ($s) {
   static $n = 0;
   $n++;
   $s = $n.':'.$s;
   $this-> foo($x[0].':'.$s);
};

As for simply omitting the keyword, e.g. function () () - as already
suggested: I don't like that syntax either. Although I'm not a fan of
too much language verbosity (that's why I don't like Fortran, Basic and
Pascal), I think in this case, a little more verbosity wouldn't hurt -
and typing 'use' is just 3 additional characters.

Now for the examples for the smaller issues:

Segfault:

<?php

$a = function () {
   $GLOBALS['a'] = NULL;
   echo "destroyed closure\n";
};

var_dump ($a);
$a ();
?> 

This crashes - due to the fact that the currently used op_array is
destroyed upon destruction of the variable. This could get even more
interesting if the closure called itself recursively. My proposal is to
create a copy (but not a reference, just do a normal copy, for resources
or objects that will just do the trick) of the variable internally in
zend_call_function and zend_do_fcall_common_helper into a dummy zval and
destroy that zval after the function call ended. That way, the GC won't
kick in until after the execution of the closure. In zend_call_function
that's easy - in zend_do_fcall_common helper we have the problem that
the variable containing the closure is no longer available. An idea
could be that the INIT_FCALL functions always additionally push the
lambda zval to the argument stack (inside the function it will be
ignored) and the fcall_common_helper will remove that zval from the
stack prior to returning (and free it). If a non-closure is called, NULL
(or an empty zval or whatever) could be pushed to the stack instead.
Hmm, perhap's I'll have a better idea tomorrow.

Anyway, since Andi suggested to use objects instead of resources, I'd
like to use your patch as a starting point, if there are no objections.

Regards,
Christian

-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php