Add support for non-scalar foreach keys #278

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
5 participants
Owner

nikic commented Feb 18, 2013

This is just a preliminary patch for the https://wiki.php.net/rfc/foreach-non-scalar-keys RFC.

Owner

nikic commented Feb 18, 2013

This patch basically changes the signature for get_current_key from

int (*get_current_key)(zend_object_iterator *iter, char **str_key, uint *str_key_len, ulong *int_key TSRMLS_DC);

to

zval *(*get_current_key)(zend_object_iterator *iter TSRMLS_DC);

and does the necessary changes in all extensions (hopefully I didn't miss any ^^).

The get_current_key handler returns a zval* with already increased refcount. The return value may be NULL if and only if an exception was thrown. (Maybe this exception should be removed too, because right now I'm not sure this case is handled properly everywhere.)

Things still to do: Figure out what to do with non-string/int keys when converting to array (iterator_to_array / CacheIterator).

Use PHP's array behavior for iterator_to_array and CachingIterator
This commit adds a new ZEND_API function array_set_zval_key, that sets
the key in the zval with the same semantics and errors as PHP would
normally do. This new function is used to implement iterator_to_array
and CachingIterator for non-scalar keys. In particular
iterator_to_array() should now behave exactly the same as doing a
manual loop with array insertion:

    foreach ($it as $k => $v) {
	    $array[$k] = $v;
    }
Contributor

datibbaw commented Feb 28, 2013

Awesome work! 👍

wpajqz commented Feb 28, 2013

good

发自我的小米手机

datibbaw notifications@github.com编写:

Awesome work! 👍


Reply to this email directly or view it on GitHub:
#278 (comment)

Contributor

weltling commented Mar 1, 2013

Which tests can/should I run to test the patch?

Owner

nikic commented Mar 2, 2013

@weltling Zend/tests and ext/spl/tests should cover the main thing. Though this change is mostly changing the use of the API in extensions all over the place, so "all of them" might be better :)

Contributor

weltling commented Mar 2, 2013

@nikic Ok, thought that :) I was just wondering there are no tests for the special case mentioned in the RFC. Performed them altogether now, looks good.

@@ -1171,6 +1171,27 @@ ZEND_API int zend_hash_get_current_key_ex(const HashTable *ht, char **str_index,
return HASH_KEY_NON_EXISTANT;
}
+ZEND_API zval *zend_hash_get_current_key_zval_ex(const HashTable *ht, HashPosition *pos) {
@reeze

reeze Mar 7, 2013

Contributor

currently, zend_hash.c didn't depend on zval struct, will it better move the function to zend_API ?

Owner

nikic commented Mar 12, 2013

Merged in fcc6611.

@nikic nikic closed this Mar 12, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment