Hi Christian,

I'm fine with your suggestion for lexical variables syntax, but I don't
know if we really need brackets around them. For now I changed syntax in
the following way.

$func = function ($x, $y) use $a, $b, $c {
}

According to segfault, I added a check that emits fatal error.

I don't like to use separate HashTable for lexical variables, because
- it takes memory (however it won't be used for all regular op_arrays)
- it requires new special modifier for FETCH opcode
- opcode caches must check for this additional table and copy it

My idea with usage of static_variables doesn't require any opcode cache
modification at all.

I'm fine if you'll improve my patch (It's mainly yours :)

Thanks. Dmitry.


Christian Seiler wrote:
>  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