Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:55621
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain uw.no from 193.71.32.2 cause and error)
Message-ID: <4E7CD562.7040201@uw.no>
Date: Fri, 23 Sep 2011 20:52:18 +0200
User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.21) Gecko/20110831 Thunderbird/3.1.13
MIME-Version: 1.0
To: Etienne Kneuss <colder@php.net>
CC: internals@lists.php.net
References: <4E7C764C.3000808@uw.no> <CACQesk7q+PXPd3hVNY5nvar8JvWRTCzjST7kW_cRKMAtV7XX2g@mail.gmail.com> <4E7C8C78.3060608@uw.no>
In-Reply-To: <4E7C8C78.3060608@uw.no>
Content-Type: multipart/mixed;
 boundary="------------010408020602090107000506"
Subject: Re: [PHP-DEV] [PATCH] Fix for bug #55754
From: dk@uw.no ("Daniel K.")

--------------010408020602090107000506
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 09/23/2011 03:41 PM, Daniel K. wrote:
> On 09/23/2011 02:28 PM, Etienne Kneuss wrote:
>> On Fri, Sep 23, 2011 at 14:06, Daniel K. <dk@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... ?
> 
> [*SNIP Lengthy description of why the patch is right*]
> 
>> 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()

However, it can be done with less complexity, we only have to make sure
that the function arguments that the lexer sends to zend_do_pass_param
is not promoted to be sent by reference if the lexer already has decided
that you did not specify a variable. See: Zend/zend_language_parser.y ->
non_empty_function_call_parameter_list:

In other words, only consider promoting to send by reference if the
argument passed to the function is not of type ZEND_SEND_VAL.

--- 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) && original_op != ZEND_SEND_VAL) {
 				send_by_reference = 1;
 				if (op == ZEND_SEND_VAR && zend_is_function_or_method_call(param)) {
 					/* Method call */

Thanks for questioning my patch, and making me recheck what I was doing,
so that this minimal patch could come to life.

This still passes the test-suite with flying colours.

As a bonus this version of the fix applies to both 5.3-latest and
5.4-latest.


>> 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.

I think I misunderstood you last time around.

I am of the opinion that the strict warning in question is completely
bogus. If the arginfo says: ZEND_SEND_PREFER_REF, Zend had better shut
up when a _value_ is passed.


Daniel K.

--------------010408020602090107000506
Content-Type: text/x-patch;
 name="bug55754_minimal.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="bug55754_minimal.patch"

diff --git a/Zend/zend_compile.c b/Zend/zend_compile.c
index c325a7e..d361c64 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) && original_op != ZEND_SEND_VAL) {
 				send_by_reference = 1;
 				if (op == ZEND_SEND_VAR && zend_is_function_or_method_call(param)) {
 					/* Method call */

--------------010408020602090107000506--