On Mon, Mar 11, 2013 at 8:42 PM, Dmitry Stogov <dmi...@zend.com> wrote:
> > > On Mon, Mar 11, 2013 at 10:27 PM, Nikita Popov <nikita....@gmail.com>wrote: > >> On Mon, Mar 11, 2013 at 9:50 AM, Dmitry Stogov <dmi...@zend.com> wrote: >> >>> 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). >> > > Agree. I missed it. > > >> 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. >> > > 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. > 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