On 09/23/2011 02:28 PM, Etienne Kneuss wrote:
> On Fri, Sep 23, 2011 at 14:06, Daniel K. <d...@uw.no> wrote:
>> When a built-in function is defined with ZEND_SEND_PREFER_REF, PHP will
>> issue a strict warning if you use an assignment expression as the parameter.
>
> The patch looks strange to me, why would you only consider stuff like
> foo($a = 2) ? what about passing any other expressions:
> foo(array(..)), foo(funcNotReturningARef()) etc... ?
foo(array(..)) is not a problem, as the op_type of 'array(..)' in this
context is IS_TMP_VAR, and thus not a candidate for passing by
reference, as far as zend_do_pass_param() is concerned.
For foo(funcNotReturningARef()) the lexer sets EA.type for the value of
funcNotReturningARef() to ZEND_PARSED_FUNCTION_CALL which in turn gets
checked in zend_do_pass_param(), just a few lines below where I added
the check to avoid the warning for assignment expressions (see below).
If ZEND_PARSED_FUNCTION_CALL is found, ZEND_ARG_SEND_SILENT is
eventually added to the opline, which in turn is checked for in
zend_vm_execute.c (ZEND_SEND_VAR_NO_REF_SPEC_VAR_HANDLER) where the
Strict warning is suppressed if ZEND_ARG_SEND_SILENT is set.
We could of course use the same mechanism, to just silence the warning,
but then the temporary variable holding the result of the assignment
expression would still be sent by reference, which I consider a bug.
I believe the right thing would be to pass it by value, to avoid nasty
surprises if any C code were to check if the argument was sent by
reference, and try to act accordingly.
> To me it makes not much sense to distinguish different non-ref
> expressions in that regard, they should all be handled the same.
It makes very much sense to make the distinction, as not all expressions
are made the same. Only IS_VAR and IS_CV ones are considered candidates
for passing by reference in zend_do_pass_param()
> Whether we want the actual error on some of our functions like
> current()/end() etc.. is another question, but that should be fixed at
> a totally different level.
This might very well be, PHP internals is not my first language, and I
might be completely off the track here. Do you by any chance have an
opinion as to where this 'totally different level' might be?
>> diff --git a/Zend/zend_compile.c b/Zend/zend_compile.c
>> index c325a7e..df30d3d 100644
>> --- a/Zend/zend_compile.c
>> +++ b/Zend/zend_compile.c
>> @@ -2096,7 +2096,7 @@ void zend_do_pass_param(znode *param, zend_uchar op,
>> int offset TSRMLS_DC) /* {{
>>
>> if (function_ptr) {
>> if (ARG_MAY_BE_SENT_BY_REF(function_ptr, (zend_uint) offset))
>> {
>> - if (param->op_type & (IS_VAR|IS_CV)) {
>> + if (param->op_type & (IS_VAR|IS_CV) &&
>> param->u.EA.type != ZEND_PARSED_EXPR_NO_PASS_BY_REF) {
>> send_by_reference = 1;
>> if (op == ZEND_SEND_VAR &&
>> zend_is_function_or_method_call(param)) {
>> /* Method call */
> op = ZEND_SEND_VAR_NO_REF;
> send_function =
> ZEND_ARG_SEND_FUNCTION | ZEND_ARG_SEND_SILENT;
zend_is_function_or_method_call(param) returns true if
ZEND_PARSED_FUNCTION_CALL is set by the lexer, and then the warning is
silenced by virtue of adding ZEND_ARG_SEND_SILENT to send_function.
I think my method, to set ZEND_PARSED_EXPR_NO_PASS_BY_REF is in the same
vein as the similar existing method, and sufficient to avoid the buggy
behaviour.
Daniel K.
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
