Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:38646
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: unknown (pb1.pair.com: domain php.net does not designate 85.214.94.56 as permitted sender)
Date: Thu, 26 Jun 2008 21:48:57 +0200
Reply-To: Marcus Boerger <helly@php.net>
Message-ID: <322121864.20080626214857@marcus-boerger.de>
To: Christian Seiler <chris_se@gmx.net>
CC: Dmitry Stogov <dmitry@zend.com>, php-dev List <internals@lists.php.net>, 
	Andi Gutmans <andi@zend.com>, Stanislav Malyshev <stas@zend.com>
In-Reply-To: <4863C299.1080002@gmx.net>
References: <4856A547.3080801@gmx.net> <485A35A0.9050602@zend.com> <485AF253.2070400@gmx.net> <485B908D.7000106@zend.com> <4863C299.1080002@gmx.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
Subject: Re: [PHP-DEV] [PATCH] [RFC] Closures and lambda functions in PHP
From: helly@php.net (Marcus Boerger)

Hello Christian,

Thursday, June 26, 2008, 6:23:53 PM, you wrote:

> Hi Dmitry,

>> I'm fine if you'll improve my patch (It's mainly yours :)

> I updated my closures RFC: http://wiki.php.net/rfc/closures

> I have based my new version of the patch on yours (Dmitry), but I made
> some changes to that:

>   * Objects instead of resources are used, two new files
>     zend_closures.[ch] are added where the new Closure class
>     is defined. Currently, it contains a dummy __toString method
>     that in future may be extended to provide enhanced debugging info,
>     also further additional cool stuff could be added to such a
>     class later on. But I prefer to only add the basic closure
>     functionality at first - you can always extend it once it's there.

>   * I have *not* added any __invoke() magic to normal objects. This is
>     mainly due to the simple reason that adding that would not help
>     a closure implementation at all. Closures need some engine internal
>     magic (use a dynamically created op_array instead of looking one up,
>     setting the correct class scope and setting the correct EG(this). And
>     as I said: I want to stick with the closure basics for now.

>     That said, I do like the possibility of invoking objects directly, so
>     I suggest someone created an additional proposal for that?

>   * I've added a patch for PHP HEAD (PHP 6.0). This is due to the fact
>     that Dmitry's variant of my patch has far less intersections with
>     the unicode functionality than my original patch, so it was quite
>     straight-forward to do so.

>   * Lexical vars are now copied instead of referenced by default. Using
>     & in front of the var, the behaviour may be changed. I added that in
>     order to demonstrate that both was possible and that a simply change
>     of grammar suffices. In my eyes this is the main issue where a
>     discussion has to take place (i.e. copy or reference by default?
>     possibility to change default via syntax? which lexical syntax?)
>     before the proposal can be accepted.

>   * I provided patches for both lexical $var and use ($var) syntaxes.

>   * I provided a patch variant that only stores $this if $this is
>     explicitely used inside a closure (or a nested closure of that
>     closure). This works since it is possible to detect whether $this
>     is used at compile time. For this, I have added a this_used flag
>     to the op_array structure.

>   * I added tests (Zend/tests/closures_*.phpt) that ensure the correct
>     behaviour of closures.

> [Note that I created my own local SVN repos for developing these patches
> because I was fed up with CVS's inability to local diffs and locally
> mark files as added to include them in the diffs. Just to explain the
> format of the patch.]

> Anyway, feel free to discuss.

> In my eyes, the following questions should be answered:

>   * Do you want closures in PHP?

>     I have not seen a single negative reaction to my proposal, so I
>     assume the answer to that is yes. ;-)

yes

>   * Which syntax should be used for lexical variables? Should references
>     or copies be created by default?

>     This is far trickier.

>     First of all: There must *always* be the _possiblity_ to create
>     references, else you can't call it closures.

>     Second: I prefer the 'lexical' keyword, but I could live with the
>     use solution [function () use ($...)].

'use' becasue no new keyword has to be introduced which would brake stuff
no matter what the keyword will be.

>     Third: There are several arguments against default referencing:

>       * (use syntax) As they look similar to parameters and normal
>                      parameters aren't passed by ref, this could be quite
>                      odd.
>       * Loop index WTFs (see proposal)
>       * Speed? (You always read that refs are slower in PHP than normal
>         variable copies. Is that actually true?)
>       * While it is possible to simply add an & in front of the variable
>         name to switch to refs in "no refs default" mode, there is no
>         obvious syntax to use copies in "refs default" mode other than
>         unsetting the variable in the parent scope immediately after
>         closure creation.

I like the new ability to reference if wanted. But then I don't like
references at all. Along with the fact that in PHP objects are always
references I slightly tend to not want reference functionality. Since it
is handled in the parser you could submit with either version and check
during evaluation periode if people disagree with your choice - or the
list choice if that's what decides.

>     Fourth: There are several arguments for default referencing:

>       * (lexical syntax) global also creates a reference, why shouldn't
>                          lexical?
>       * Other languages *appear* to exhibit a very similar behaviour to
>         PHP if PHP created references. This is due to the fact that
>         other languages have a different concept of scope as PHP
>         does.

>     Although the list of against arguments appears to be longer, I do
>     prefer using references by default nevertheless. But that's just
>     my personal opinion.

>   * Are you OK with the change that $this is only stored when needed?

>     I don't see a problem. Dmitry seems to be very touchy (;-)) about
>     changing op_arrays but in this case it's only a flag so I don't
>     see a problem for opcode caches (in contrast to a HashTable where
>     the opcode cache must actually add code to duplicate that table).

I see it dangerous.... eval comes to mind. And also, why create something
special when the normal way of doing things that is done everywhere else
would work too.

>   * Do you want closures in PHP 5.3?

>     Since the majority of core developers appear to be against it, I
>     presume the answer is no.

I am still in favor it and against a 5.4.

> I will provide a revised patch that incorporates the results of the
> following discussion for 5_3 and HEAD once consensus or at least a
> majority regarding the remaining issues is reached. I will also rewrite
> the proposal to reflect the discussion results and adjust the tests.
> After that, I hope that someone will commit at least the HEAD version.

Comments on the first patch version:

- coooool, I am the listed author: Zend/zend_closures.c/h

- you shouldn't be having a __destruct. Can you prevent that?

- please drop __toString, with the new behavior only stuff that has
  something to say in a string context should have a __toString

- a tiny optimization:
+ZEND_API zend_closure *zend_get_closure(zval *obj TSRMLS_DC) /* {{{ */
+{
+       zend_class_entry *ce = Z_OBJCE_P(obj);
+       if (instanceof_function(ce, zend_ce_closure TSRMLS_CC)) {
+               zend_closure *closure = (zend_closure *)zend_object_store_get_object(obj);
+               if (closure->initialized) return closure;
+       }
+       return NULL;
+}

- a faster way would be to:
  a) add a new type (probably not so good though)
  b) add a new ce_flag
+#define ZEND_ACC_CLOSURE ...
+ZEND_API zend_closure *zend_get_closure(zval *obj TSRMLS_DC) /* {{{ */
+{
+       zend_class_entry *ce = Z_OBJCE_P(obj);
+       if ((ce->ce_flags & ZEND_ACC_CLOSURE) != 0) {
+               zend_closure *closure = (zend_closure *)zend_object_store_get_object(obj);
+               if (closure->initialized) return closure;
+       }
+       return NULL;
+}

- maybe even inline this function?

- very high quality work! thanks a lot! very well thought out and nicely
  tested even

Best regards,
 Marcus