Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:66577
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: unknown (pb1.pair.com: domain zend.com does not designate 209.85.214.169 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <CA+9eiLuV_af9wAS_1zd_NYw=NXo1kJs990s5+bb_ZQ6h8=yW4Q@mail.gmail.com>
References: <CAF+90c-E7n-WZcqNg28bX+G7zgbzhKoNCwR6hf++tfAgSzmtTw@mail.gmail.com>
	<CAF+90c-wSqeff_2oY8HeY+gDn-ZnwJeXKQ+cz-1W6xcEBjs+xw@mail.gmail.com>
	<CA+9eiLtiOcFuxRXhpGXe5b9SpyVPB1_ixJ4t1GQ3J1bvBURMKA@mail.gmail.com>
	<CAF+90c92AwjX7Niro_tT5vOv+vHeY-AmgfizL5f9i1FYhKTz2A@mail.gmail.com>
	<CA+9eiLsWNV7H1zAr1U-wPafz9SzB=eR_C33RQKgqy1Tx1+X8gw@mail.gmail.com>
	<CAF+90c_Nt=Q-vTHO3iqC24mkwULMvV+KxpYpy1ug5=7em-x1eg@mail.gmail.com>
	<CA+9eiLuV_af9wAS_1zd_NYw=NXo1kJs990s5+bb_ZQ6h8=yW4Q@mail.gmail.com>
Date: Mon, 11 Mar 2013 12:50:13 +0400
Message-ID: <CA+9eiLvnmHA7fQonaLj-oRWp_d7HmEbtoejfqWrKQxK-DOTjMg@mail.gmail.com>
To: Nikita Popov <nikita.ppv@gmail.com>
Cc: PHP internals <internals@lists.php.net>, Zeev Suraski <zeev@zend.com>, Andi Gutmans <andi@zend.com>
Content-Type: multipart/alternative; boundary=e89a8fb1f04ce8f8e404d7a242ce
Subject: Re: [PHP-DEV] Re: [VOTE] Allow non-scalar keys in foreach
From: dmitry@zend.com (Dmitry Stogov)

--e89a8fb1f04ce8f8e404d7a242ce
Content-Type: text/plain; charset=UTF-8

Hi Nikita,

The patch looks good. I have just few comments

- In ZEND_FE_FETCH handler PLAIN_OBJECT may have only STRING keys. I didn't
get why you added unreachable code for INT and NULL.

- At first, I fought, that it might be a good idea to change
zend_user_it_get_current_key() to return SUCCESS/FAILURE instead of
returning IS_NULL that has a special meaning. But after looking into the
FE_FETCH code before the commit I understood where this NULL came from. I
know that NULL key may not appear for plain array and objects but I'm not
sure about iterators and generators. Now IS_NULL keys may mean that
iterator returned it directly IS_NULL or may be it was returned because of
some error conditions. Probably it's not a problem. What do you think?

I personally, irrelevant to this patch.
I didn't found serious technical issues, so I think it may be accepted.

Thanks. Dmitry.




On Mon, Mar 11, 2013 at 10:35 AM, Dmitry Stogov <dmitry@zend.com> wrote:

> Hi Nikita,
>
> Thanks. I'll review it today.
>
> Dmitry.
>
>
> On Sun, Mar 10, 2013 at 1:47 AM, Nikita Popov <nikita.ppv@gmail.com>wrote:
>
>> On Wed, Mar 6, 2013 at 6:28 PM, Dmitry Stogov <dmitry@zend.com> wrote:
>>
>>>
>>>> I wonder what would be a good way to avoid allocating a temporary zval
>>>> for the key and freeing it again. Do you think it would be okay to pass
>>>> &EX_T((opline+1)->result.var).tmp_var into ->get_current_key() so the value
>>>> can be written directly into it without doing extra allocs/frees?
>>>>
>>>>
>>> I'm not sure it'll work. TMP_VARs don't initialize refcount, they can't
>>> be referenced or marked as a possible root of garbage.
>>> I took only a very quick look over the patch and didn't understand all
>>> the details, but probably it must be possible to copy iterator key into
>>> TMP_VAR
>>> and call copy_ctor().
>>>
>>> Please, let me review the patch when it's ready (I won't be available on
>>> March 8 and weekend).
>>>
>>> Thanks. Dmitry.
>>>
>>
>> Here is the new patch:
>> https://github.com/nikic/php-src/commit/a1bfc8105713eeb4e66e852b81884b567ad56020
>> It passes in the tmp_var in as a zval*, which can then be set using the
>> ZVAL_* macros (basically the same way as it's done with return_value). This
>> way we don't need any further zval allocs and frees. It also turned out
>> that doing it this way is more convenient to use in the respective key
>> handlers.
>>
>> Nikita
>>
>
>

--e89a8fb1f04ce8f8e404d7a242ce--