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 <dmi...@zend.com> wrote: > Hi Nikita, > > Thanks. I'll review it today. > > Dmitry. > > > On Sun, Mar 10, 2013 at 1:47 AM, Nikita Popov <nikita....@gmail.com>wrote: > >> On Wed, Mar 6, 2013 at 6:28 PM, Dmitry Stogov <dmi...@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 >> > >
