Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:38649
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmx.net designates 213.165.64.20 as permitted sender)
Message-ID: <486400A5.1060009@gmx.net>
Date: Thu, 26 Jun 2008 22:48:37 +0200
User-Agent: Thunderbird 2.0.0.14 (X11/20080421)
MIME-Version: 1.0
To: Marcus Boerger <helly@php.net>
CC: Dmitry Stogov <dmitry@zend.com>, 
 php-dev List <internals@lists.php.net>,
 Andi Gutmans <andi@zend.com>, Stanislav Malyshev <stas@zend.com>
References: <4856A547.3080801@gmx.net> <485A35A0.9050602@zend.com> <485AF253.2070400@gmx.net> <485B908D.7000106@zend.com> <4863C299.1080002@gmx.net> <322121864.20080626214857@marcus-boerger.de>
In-Reply-To: <322121864.20080626214857@marcus-boerger.de>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Subject: Re: [PHP-DEV] [PATCH] [RFC] Closures and lambda functions in PHP
From: chris_se@gmx.net (Christian Seiler)

Hi Marcus,

> I like the new ability to reference if wanted. But then I don't like
> references at all.

As I said: Without reference support, you can't call it closures.
Closures must per definition have the possibility to change the values
of used variables in the parent scope - and the only sensible PHP way to
do that is references.

If people here say: We want copy as default and references optional,
that's fine with me since real closures can still be achieved and -
let's face it - since PHP wasn't designed with closures in mind, the
syntax will always be not 100% perfect, regardsless of how we'll do it.
But I'm stronly against removing reference support entirely.

>>   * 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.

Certain things don't work anyway as of now, see bug
http://bugs.php.net/bug.php?id=43163 for example... And that's intended
behaviour.

Ok, eval() inside a closure still is a possible problem but we still
could say "ok, as soon as eval() appears, just assume $this is used".
That will benefit those people who use neither $this nor eval() inside a
closure with an optimization ($this will be GCed and not copied). And as
far as I can see, eval() is the only thing you can do inside a normal
class method that will allow you to access $this without the compiler
knowing about it. But thanks for pointing eval out, I hadn't thought of
that.

> And also, why create something
> special when the normal way of doing things that is done everywhere else
> would work too.

What do you mean by that?

> Comments on the first patch version:
> 
> - coooool, I am the listed author: Zend/zend_closures.c/h

Oh, yeah, I just copied some header I found elsewhere. Should I put my
name there or what's the policy for contributions?

> - you shouldn't be having a __destruct. Can you prevent that?

But if I don't have a destructor, how do I garbage collect the op_array?

> - please drop __toString, with the new behavior only stuff that has
>   something to say in a string context should have a __toString

Somebody on the internals list suggested adding it to print out useful
debugging info. But I'm fine with dropping it until there is actually a
useful implementation.

> - 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;
> +}

Yes, good idea.

> - 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 ...

I'm quite indifferent to that change. On the one hand, it's a
performance optimization, on the other hand, it digs deeper into current
Zend code.

> - maybe even inline this function?

Yes, good idea.

As stated before, I'll keep all changes in mind and post and updated
patch as soon as the discussion is done.

Regards,
Christian