# [VOTE] Allow non-scalar keys in foreach

- Thread: https://externals.io/message/66295
- Original archive: https://news-web.php.net/php.internals/66295
- Started by Nikita Popov on 2013-02-27T19:33:27+00:00
- Messages: 13
- Reply by email to internals@lists.php.net
- More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt

---

## #66295 — Nikita Popov — 2013-02-27T19:33:27+00:00

URL: https://externals.io/message/66295#66295

Hi internals!

I've opened the voting the the foreach-keys RFC:

https://wiki.php.net/rfc/foreach-non-scalar-keys#vote

The discussion for this RFC can be found here:
http://markmail.org/message/rzoacqaesxbd67lu

Thanks,
Nikita

---

## #66494 — Nikita Popov — 2013-03-06T15:27:45+00:00

URL: https://externals.io/message/66295#66494 · In reply to: #66295

> Hi internals!
>
> I've opened the voting the the foreach-keys RFC:
>
> https://wiki.php.net/rfc/foreach-non-scalar-keys#vote
>
> The discussion for this RFC can be found here:
> http://markmail.org/message/rzoacqaesxbd67lu
>

The RFC was accepted unanimously, with 21 votes in favor. I'll merge the
patch sometime soon.

Thanks,
Nikita

---

## #66496 — Dmitry Stogov — 2013-03-06T16:41:19+00:00

URL: https://externals.io/message/66295#66496 · In reply to: #66494

Hi Nikita,

few notes about the patch...

- you may avoid estrndup() in zend_hash_current_key_zval_ex() for interned
strings.

- I didn't completely get why did you change the "key" operand type from
IS_TMP_VAR to IS_VAR and how it affects performance
As I understood now you need to allocate new zval on each loop iteration
even for foreach over plain arrays. :(

Thanks. Dmitry.



> On Wed, Feb 27, 2013 at 8:33 PM, Nikita Popov <nikita.ppv@gmail.com>
> wrote:
>
> > […]
>
> The RFC was accepted unanimously, with 21 votes in favor. I'll merge the
> patch sometime soon.
>
> Thanks,
> Nikita

---

## #66497 — Nikita Popov — 2013-03-06T17:16:27+00:00

URL: https://externals.io/message/66295#66497 · In reply to: #66496

> Hi Nikita,
>
> few notes about the patch...
>
> - you may avoid estrndup() in zend_hash_current_key_zval_ex() for interned
> strings.
>

Good idea, I'll do that.

>
> - I didn't completely get why did you change the "key" operand type from
> IS_TMP_VAR to IS_VAR and how it affects performance
> As I understood now you need to allocate new zval on each loop iteration
> even for foreach over plain arrays. :(
>

Good point, I didn't consider the performance implication the type change
would have. The intent behind that was to avoid copying the get_current_key
zval into the temporary (and destroying it then), but I didn't consider how
it affects normal arrays. This should be changed back to TMP_VAR.

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?

Nikita

---

## #66498 — Dmitry Stogov — 2013-03-06T17:28:41+00:00

URL: https://externals.io/message/66295#66498 · In reply to: #66497

> 
>
> > […]
>
> Good idea, I'll do that.
>
> > […]
>
> Good point, I didn't consider the performance implication the type change
> would have. The intent behind that was to avoid copying the get_current_key
> zval into the temporary (and destroying it then), but I didn't consider how
> it affects normal arrays. This should be changed back to TMP_VAR.
>

It would be great. I can agree that new features may work slower, but
really don't like when they slowdown existing and mach more usual cases.


>
> 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.

---

## #66499 — Reeze Xia — 2013-03-06T18:14:06+00:00

URL: https://externals.io/message/66295#66499 · In reply to: #66498

Hi Nikita,
    I got some test failure with the patch, one test failure and some memory leaks.
see: https://gist.github.com/reeze/5101596


--  
reeze | reeze.cn

已使用 Sparrow (http://www.sparrowmailapp.com/?sig)  

在 2013年3月7日星期四，上午1:28，Dmitry Stogov 写道：

> 
>  
> > […]
>  
>  
> It would be great. I can agree that new features may work slower, but
> really don't like when they slowdown existing and mach more usual cases.
>  
>  
> > […]
>  
> 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.

---

## #66570 — Nikita Popov — 2013-03-09T21:47:56+00:00

URL: https://externals.io/message/66295#66570 · In reply to: #66498

>
> > […]
> 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

---

## #66575 — Dmitry Stogov — 2013-03-11T06:35:04+00:00

URL: https://externals.io/message/66295#66575 · In reply to: #66570

Hi Nikita,

Thanks. I'll review it today.

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

---

## #66577 — Dmitry Stogov — 2013-03-11T08:50:13+00:00

URL: https://externals.io/message/66295#66577 · In reply to: #66575

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.






> 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:
>
> > […]

---

## #66590 — Nikita Popov — 2013-03-11T18:27:24+00:00

URL: https://externals.io/message/66295#66590 · In reply to: #66577

> 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.
>

You are right about the `NULL` case, that can indeed not occur. But INT is
possible, e.g. consider this:

<?php
foreach ((object) ['x','y','z'] as $k => $v) {
    var_dump($k);
}

this will give you keys int(0), int(1), int(2).

I'll remove the check for 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?
>

In foreach IS_NULL can't occur in an error condition, because the loop is
already aborted when get_current_data provides NULL. IS_NULL can only
happen when its explicitly provided (or handlers are incorrectly coded). I
think the IS_NULL fallback is mainly important when the iterator is used
for other things (like a dual it), to be sure that it'll always work. I
don't think that it's important to distinguish between explicit `NULL` and
failure NULL.

I have one more question: Right now I added the
zend_hash_get_current_key_zval API in zend_hash.c/h, which is a bit ugly
because it has a zval dependency (unlike all the other code in there) and
requires me to forward-declare the zval struct. Would it be better to move
this somewhere else, e.g. zend_API.c/h? If so, can I still keep the same
name (with the zend_hash_ prefix)? If I move it to zend_API, I won't be
able to do the IS_CONSISTENT check anymore, is that a problem?

Thanks,
Nikita

---

## #66591 — Dmitry Stogov — 2013-03-11T19:42:18+00:00

URL: https://externals.io/message/66295#66591 · In reply to: #66590

> 
>
> > […]
>
> You are right about the `NULL` case, that can indeed not occur. But INT is
> possible, e.g. consider this:
>
> <?php
> foreach ((object) ['x','y','z'] as $k => $v) {
>     var_dump($k);
> }
>
> this will give you keys int(0), int(1), int(2).
>

Agree. I missed it.


> I'll remove the check for NULL.
>
>
> - At first, I fought, that it might be a good idea to change
> > […]
>
> In foreach IS_NULL can't occur in an error condition, because the loop is
> already aborted when get_current_data provides NULL. IS_NULL can only
> happen when its explicitly provided (or handlers are incorrectly coded). I
> think the IS_NULL fallback is mainly important when the iterator is used
> for other things (like a dual it), to be sure that it'll always work. I
> don't think that it's important to distinguish between explicit `NULL` and
> failure NULL.
>

Agree as well.


> I have one more question: Right now I added the
> zend_hash_get_current_key_zval API in zend_hash.c/h, which is a bit ugly
> because it has a zval dependency (unlike all the other code in there) and
> requires me to forward-declare the zval struct. Would it be better to move
> this somewhere else, e.g. zend_API.c/h? If so, can I still keep the same
> name (with the zend_hash_ prefix)? If I move it to zend_API, I won't be
> able to do the IS_CONSISTENT check anymore, is that a problem?
>

I think we can move zval forward declaration (typedef struct _zval_struct
zval;) from zend.h into zend_type.h.

Thanks. Dmitry.

---

## #66596 — Nikita Popov — 2013-03-12T16:43:30+00:00

URL: https://externals.io/message/66295#66596 · In reply to: #66591

>
>
> On Mon, Mar 11, 2013 at 10:27 PM, Nikita Popov <nikita.ppv@gmail.com>wrote:
>
> > […]
>
> Agree. I missed it.
>
>
> > […]
>
> Agree as well.
>
>
> > […]
>
> I think we can move zval forward declaration (typedef struct _zval_struct
> zval;) from zend.h into zend_type.h.
>

I now merged the changeset in
https://github.com/php/php-src/commit/fcc6611de9054327441786e52444b5f8eecdd525(for
PHP-5.5 and master) with the forward declaration moved. Also updated
some array functions to use the new get_current_key_zval function :)

Nikita

---

## #66597 — Dmitry Stogov — 2013-03-12T17:18:25+00:00

URL: https://externals.io/message/66295#66597 · In reply to: #66596

Hi Nikita,

I suppose it must fine now, but let me take a quick look tomorrow morning.

Thanks. Dmitry.



> 
>
> > […]
>
> I now merged the changeset in
> https://github.com/php/php-src/commit/fcc6611de9054327441786e52444b5f8eecdd525(for PHP-5.5 and master) with the forward declaration moved. Also updated
> some array functions to use the new get_current_key_zval function :)
>
> Nikita