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
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
