Re: [PHP-DEV] Re: [VOTE] Allow non-scalar keys in foreach

2013-03-12 Thread Nikita Popov
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.comwrote:

 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


Re: [PHP-DEV] Re: [VOTE] Allow non-scalar keys in foreach

2013-03-12 Thread Dmitry Stogov
Hi Nikita,

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

Thanks. Dmitry.

On Tue, Mar 12, 2013 at 8:43 PM, Nikita Popov nikita@gmail.com wrote:

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

 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



Re: [PHP-DEV] Re: [VOTE] Allow non-scalar keys in foreach

2013-03-11 Thread Dmitry Stogov
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



Re: [PHP-DEV] Re: [VOTE] Allow non-scalar keys in foreach

2013-03-11 Thread Dmitry Stogov
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.comwrote:

 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





Re: [PHP-DEV] Re: [VOTE] Allow non-scalar keys in foreach

2013-03-11 Thread Nikita Popov
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).

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


Re: [PHP-DEV] Re: [VOTE] Allow non-scalar keys in foreach

2013-03-11 Thread Dmitry Stogov
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.


Re: [PHP-DEV] Re: [VOTE] Allow non-scalar keys in foreach

2013-03-09 Thread Nikita Popov
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


[PHP-DEV] Re: [VOTE] Allow non-scalar keys in foreach

2013-03-06 Thread Nikita Popov
On Wed, Feb 27, 2013 at 8:33 PM, Nikita Popov nikita@gmail.com wrote:

 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


Re: [PHP-DEV] Re: [VOTE] Allow non-scalar keys in foreach

2013-03-06 Thread Dmitry Stogov
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, Mar 6, 2013 at 7:27 PM, Nikita Popov nikita@gmail.com wrote:

 On Wed, Feb 27, 2013 at 8:33 PM, Nikita Popov nikita@gmail.com
 wrote:

  Hi internals!
 interned strings
  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



Re: [PHP-DEV] Re: [VOTE] Allow non-scalar keys in foreach

2013-03-06 Thread Nikita Popov
On Wed, Mar 6, 2013 at 5:41 PM, Dmitry Stogov dmi...@zend.com wrote:

 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


Re: [PHP-DEV] Re: [VOTE] Allow non-scalar keys in foreach

2013-03-06 Thread Dmitry Stogov
On Wed, Mar 6, 2013 at 9:16 PM, Nikita Popov nikita@gmail.com wrote:

 On Wed, Mar 6, 2013 at 5:41 PM, Dmitry Stogov dmi...@zend.com wrote:

 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.


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.


[PHP-DEV] 回复: [PHP-DEV] Re: [VOTE] Allow non-scalar keys in foreach

2013-03-06 Thread Reeze Xia
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 写道:

 On Wed, Mar 6, 2013 at 9:16 PM, Nikita Popov nikita@gmail.com 
 (mailto:nikita@gmail.com) wrote:
  
  On Wed, Mar 6, 2013 at 5:41 PM, Dmitry Stogov dmi...@zend.com 
  (mailto:dmi...@zend.com) wrote:
   
   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.
   
  
  
 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.