Re: [PATCH] [RFC] Closures and lambda functions in PHP

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