Newsgroups: php.internals Path: news.php.net Xref: news.php.net php.internals:38438 Return-Path: Mailing-List: contact internals-help@lists.php.net; run by ezmlm Delivered-To: mailing list internals@lists.php.net Received: (qmail 47402 invoked from network); 19 Jun 2008 23:58:03 -0000 Received: from unknown (HELO lists.php.net) (127.0.0.1) by localhost with SMTP; 19 Jun 2008 23:58:03 -0000 Authentication-Results: pb1.pair.com header.from=chris_se@gmx.net; sender-id=pass Authentication-Results: pb1.pair.com smtp.mail=chris_se@gmx.net; spf=pass; sender-id=pass Received-SPF: pass (pb1.pair.com: domain gmx.net designates 213.165.64.20 as permitted sender) X-PHP-List-Original-Sender: chris_se@gmx.net X-Host-Fingerprint: 213.165.64.20 mail.gmx.net Linux 2.5 (sometimes 2.4) (4) Received: from [213.165.64.20] ([213.165.64.20:59592] helo=mail.gmx.net) by pb1.pair.com (ecelerity 2.1.1.9-wez r(12769M)) with ESMTP id 9A/A0-57468-982FA584 for ; Thu, 19 Jun 2008 19:58:02 -0400 Received: (qmail invoked by alias); 19 Jun 2008 23:57:58 -0000 Received: from p54A16776.dip.t-dialin.net (EHLO chris-se.dyndns.org) [84.161.103.118] by mail.gmx.net (mp020) with SMTP; 20 Jun 2008 01:57:58 +0200 X-Authenticated: #186999 X-Provags-ID: V01U2FsdGVkX1/aaC9knQjaBKbMxuo5LWHqJvzNFT2s1Ql4hgHKpD HbSmY54j1XpX5/ Received: from [192.168.0.175] (HSI-KBW-091-089-005-213.hsi2.kabelbw.de [91.89.5.213]) by chris-se.dyndns.org (Postfix) with ESMTP id 2F6B210664; Fri, 20 Jun 2008 01:44:36 +0200 (CEST) Message-ID: <485AF253.2070400@gmx.net> Date: Fri, 20 Jun 2008 01:57:07 +0200 User-Agent: Thunderbird 2.0.0.14 (X11/20080421) MIME-Version: 1.0 To: Dmitry Stogov CC: php-dev List , Andi Gutmans , Stanislav Malyshev References: <4856A547.3080801@gmx.net> <485A35A0.9050602@zend.com> In-Reply-To: <485A35A0.9050602@zend.com> X-Enigmail-Version: 0.95.6 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Y-GMX-Trusted: 0 Subject: Re: [PHP-DEV] [PATCH] [RFC] Closures and lambda functions in PHP From: chris_se@gmx.net (Christian Seiler) 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: 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