Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:38319
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: <48578FD2.8030307@gmx.net>
Date: Tue, 17 Jun 2008 12:20:02 +0200
User-Agent: Thunderbird 2.0.0.14 (X11/20080421)
MIME-Version: 1.0
To: php-dev List <internals@lists.php.net>
References: <4856A547.3080801@gmx.net> <1961603263.20080617120320@marcus-boerger.de>
In-Reply-To: <1961603263.20080617120320@marcus-boerger.de>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Subject: Re: [PHP-DEV] [PATCH] [RFC] Closures and lambda functions in PHP
From: chris_se@gmx.net (Christian Seiler)

Hi Marcus,

> very nice work.

Thanks!

> The only thing I don't like is the function naming
> (“\0__compiled_lambda_FILENAME_N”). Can we drop the \0?

I used \0 because it is already used in two other places:

  1) create_function (run-time lambda functions) uses \0__lambda_N
  2) build_runtime_defined_function_key uses \0 to start function names.

I can drop it if you like; personally, I don't care for either solution
- it's an internal name that *may* leak to userspace in some
circumstances but is never really useful for userspace anyway..

A minor side-note here: I oriented myself at
build_runtime_defined_function_key at the time of writing but I have
noticed a slight discrepancy between function names generated by
build_runtime_defined_function_key and the normal function names: When
stored in the corresponding function_table hash, for runtime defined
function keys it is opline->op1.u.constant.value.str.len, whereas for
normal function names it is »Z_STRLEN_P(...) + 1« and thus including the
*trailing* (not preceding!) \0 in the hash key for normal function names
but not including it for runtime defined function keys. Any idea why
that is the case? [For the record: I'm refering to the code that is
already used in PHP, not to my patch!]

> Or did you use the \0 prefix to prevent direct invocations?

No, direct invocations are prevented by the is_lambda == 1 &&
is_closure == 0 check.

> I think the best option would be to force lambda functions being public
> always.

They are. If you look at my modified version of
zend_do_begin_function_declaration, you will see that:

if (is_lambda && CG(active_class_entry)) {
	is_method = 1;
	fn_flags = ZEND_ACC_PUBLIC;
	if (CG(active_op_array)->fn_flags & ZEND_ACC_STATIC) {
		fn_flags |= ZEND_ACC_STATIC;
	}
} else if (is_lambda) {
	fn_flags = 0;
}

The only attribute that is "inherited" from the parent function is
whether that function is static or not.

> The last question is about changing visibility and overriding
> functions, do we want that, or should we mark lamdas as final?

Internal representations of lambda fuctions should never be overridden, 
so yes, ZEND_ACC_FINAL would probably be a good idea. Overriding them 
won't work anyway since the new opcode that "instantiates" a closure 
will always use the class in which the closure was defined to look it up.

I'll add that.

> Actually how does it integrate with reflection?

Good question, I will investigate that and come back to you.

> Comments about the implementation:
> 
> - ZendEngine2/zend_compile.c:
> 
>   Why provide forward declaration for free_filename(), simply putting the
>   new function above the first user avoids it and does the same with
>   increased maintainability.
> 
>   s/static void add_lexical_var (/static void add_lexical_var(/

Ok, I will fix that.

Regards,
Christian