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