From a510c7d89a350e3d885514c9a04d4cfd4af41631 Mon Sep 17 00:00:00 2001 From: Alex Dowad Date: Mon, 11 May 2020 20:32:13 +0200 Subject: [PATCH] SplFixedArray is Aggregate, not Iterable One strange feature of SplFixedArray was that it could not be used in nested foreach loops. If one did so, the inner loop would overwrite the iteration state of the outer loop. To illustrate: $spl = SplFixedArray::fromArray([0, 1]); foreach ($spl as $a) { foreach ($spl as $b) { echo "$a $b"; } } Would only print two lines: 0 0 0 1 Use the new InternalIterator feature which was introduced in ff19ec2df3 to convert SplFixedArray to an Aggregate rather than Iterable. As a bonus, we get to trim down some ugly code! Yay! --- ext/spl/spl_fixedarray.c | 211 ++++-------------- ext/spl/spl_fixedarray.stub.php | 17 +- ext/spl/spl_fixedarray_arginfo.h | 25 +-- ...xedArray_change_size_during_iteration.phpt | 43 ++++ ext/spl/tests/SplFixedArray_key_setsize.phpt | 20 -- .../tests/SplFixedArray_nested_foreach.phpt | 19 ++ .../SplFixedArray_override_getIterator.phpt | 46 ++++ ext/spl/tests/fixedarray_003.phpt | 86 ------- ext/spl/tests/fixedarray_019.phpt | 51 ----- 9 files changed, 154 insertions(+), 364 deletions(-) create mode 100644 ext/spl/tests/SplFixedArray_change_size_during_iteration.phpt delete mode 100644 ext/spl/tests/SplFixedArray_key_setsize.phpt create mode 100644 ext/spl/tests/SplFixedArray_nested_foreach.phpt create mode 100644 ext/spl/tests/SplFixedArray_override_getIterator.phpt delete mode 100644 ext/spl/tests/fixedarray_003.phpt delete mode 100644 ext/spl/tests/fixedarray_019.phpt diff --git a/ext/spl/spl_fixedarray.c b/ext/spl/spl_fixedarray.c index 915e55f17cc6..b9471d07bfa7 100644 --- a/ext/spl/spl_fixedarray.c +++ b/ext/spl/spl_fixedarray.c @@ -46,29 +46,22 @@ typedef struct _spl_fixedarray { /* {{{ */ /* }}} */ typedef struct _spl_fixedarray_object { /* {{{ */ - spl_fixedarray array; - zend_function *fptr_offset_get; - zend_function *fptr_offset_set; - zend_function *fptr_offset_has; - zend_function *fptr_offset_del; - zend_function *fptr_count; - int current; - int flags; - zend_object std; + spl_fixedarray array; + zend_function *fptr_offset_get; + zend_function *fptr_offset_set; + zend_function *fptr_offset_has; + zend_function *fptr_offset_del; + zend_function *fptr_count; + zend_object std; } spl_fixedarray_object; /* }}} */ typedef struct _spl_fixedarray_it { /* {{{ */ - zend_user_iterator intern; + zend_object_iterator intern; + int current; } spl_fixedarray_it; /* }}} */ -#define SPL_FIXEDARRAY_OVERLOADED_REWIND 0x0001 -#define SPL_FIXEDARRAY_OVERLOADED_VALID 0x0002 -#define SPL_FIXEDARRAY_OVERLOADED_KEY 0x0004 -#define SPL_FIXEDARRAY_OVERLOADED_CURRENT 0x0008 -#define SPL_FIXEDARRAY_OVERLOADED_NEXT 0x0010 - static inline spl_fixedarray_object *spl_fixed_array_from_obj(zend_object *obj) /* {{{ */ { return (spl_fixedarray_object*)((char*)(obj) - XtOffsetOf(spl_fixedarray_object, std)); } @@ -201,23 +194,17 @@ static void spl_fixedarray_object_free_storage(zend_object *object) /* {{{ */ } /* }}} */ -zend_object_iterator *spl_fixedarray_get_iterator(zend_class_entry *ce, zval *object, int by_ref); - static zend_object *spl_fixedarray_object_new_ex(zend_class_entry *class_type, zend_object *orig, int clone_orig) /* {{{ */ { spl_fixedarray_object *intern; zend_class_entry *parent = class_type; int inherited = 0; - zend_class_iterator_funcs *funcs_ptr; intern = zend_object_alloc(sizeof(spl_fixedarray_object), parent); zend_object_std_init(&intern->std, class_type); object_properties_init(&intern->std, class_type); - intern->current = 0; - intern->flags = 0; - if (orig && clone_orig) { spl_fixedarray_object *other = spl_fixed_array_from_obj(orig); spl_fixedarray_init(&intern->array, other->array.size); @@ -236,31 +223,7 @@ static zend_object *spl_fixedarray_object_new_ex(zend_class_entry *class_type, z ZEND_ASSERT(parent); - funcs_ptr = class_type->iterator_funcs_ptr; - if (!funcs_ptr->zf_current) { - funcs_ptr->zf_rewind = zend_hash_str_find_ptr(&class_type->function_table, "rewind", sizeof("rewind") - 1); - funcs_ptr->zf_valid = zend_hash_str_find_ptr(&class_type->function_table, "valid", sizeof("valid") - 1); - funcs_ptr->zf_key = zend_hash_str_find_ptr(&class_type->function_table, "key", sizeof("key") - 1); - funcs_ptr->zf_current = zend_hash_str_find_ptr(&class_type->function_table, "current", sizeof("current") - 1); - funcs_ptr->zf_next = zend_hash_str_find_ptr(&class_type->function_table, "next", sizeof("next") - 1); - } if (inherited) { - if (funcs_ptr->zf_rewind->common.scope != parent) { - intern->flags |= SPL_FIXEDARRAY_OVERLOADED_REWIND; - } - if (funcs_ptr->zf_valid->common.scope != parent) { - intern->flags |= SPL_FIXEDARRAY_OVERLOADED_VALID; - } - if (funcs_ptr->zf_key->common.scope != parent) { - intern->flags |= SPL_FIXEDARRAY_OVERLOADED_KEY; - } - if (funcs_ptr->zf_current->common.scope != parent) { - intern->flags |= SPL_FIXEDARRAY_OVERLOADED_CURRENT; - } - if (funcs_ptr->zf_next->common.scope != parent) { - intern->flags |= SPL_FIXEDARRAY_OVERLOADED_NEXT; - } - intern->fptr_offset_get = zend_hash_str_find_ptr(&class_type->function_table, "offsetget", sizeof("offsetget") - 1); if (intern->fptr_offset_get->common.scope == parent) { intern->fptr_offset_get = NULL; @@ -780,36 +743,35 @@ PHP_METHOD(SplFixedArray, offsetUnset) } /* }}} */ -static void spl_fixedarray_it_dtor(zend_object_iterator *iter) /* {{{ */ +/* {{{ Create a new iterator from a SplFixedArray instance. */ +PHP_METHOD(SplFixedArray, getIterator) { - spl_fixedarray_it *iterator = (spl_fixedarray_it *)iter; + if (zend_parse_parameters_none() == FAILURE) { + return; + } - zend_user_it_invalidate_current(iter); - zval_ptr_dtor(&iterator->intern.it.data); + zend_create_internal_iterator_zval(return_value, ZEND_THIS); } /* }}} */ -static void spl_fixedarray_it_rewind(zend_object_iterator *iter) /* {{{ */ +static void spl_fixedarray_it_dtor(zend_object_iterator *iter) /* {{{ */ { - spl_fixedarray_object *object = Z_SPLFIXEDARRAY_P(&iter->data); + zval_ptr_dtor(&iter->data); +} +/* }}} */ - if (object->flags & SPL_FIXEDARRAY_OVERLOADED_REWIND) { - zend_user_it_rewind(iter); - } else { - object->current = 0; - } +static void spl_fixedarray_it_rewind(zend_object_iterator *iter) /* {{{ */ +{ + ((spl_fixedarray_it*)iter)->current = 0; } /* }}} */ static int spl_fixedarray_it_valid(zend_object_iterator *iter) /* {{{ */ { - spl_fixedarray_object *object = Z_SPLFIXEDARRAY_P(&iter->data); + spl_fixedarray_it *iterator = (spl_fixedarray_it*)iter; + spl_fixedarray_object *object = Z_SPLFIXEDARRAY_P(&iter->data); - if (object->flags & SPL_FIXEDARRAY_OVERLOADED_VALID) { - return zend_user_it_valid(iter); - } - - if (object->current >= 0 && object->current < object->array.size) { + if (iterator->current >= 0 && iterator->current < object->array.size) { return SUCCESS; } @@ -819,122 +781,29 @@ static int spl_fixedarray_it_valid(zend_object_iterator *iter) /* {{{ */ static zval *spl_fixedarray_it_get_current_data(zend_object_iterator *iter) /* {{{ */ { - zval zindex; - spl_fixedarray_object *object = Z_SPLFIXEDARRAY_P(&iter->data); - - if (object->flags & SPL_FIXEDARRAY_OVERLOADED_CURRENT) { - return zend_user_it_get_current_data(iter); - } else { - zval *data; + zval zindex, *data; + spl_fixedarray_it *iterator = (spl_fixedarray_it*)iter; + spl_fixedarray_object *object = Z_SPLFIXEDARRAY_P(&iter->data); - ZVAL_LONG(&zindex, object->current); + ZVAL_LONG(&zindex, iterator->current); + data = spl_fixedarray_object_read_dimension_helper(object, &zindex); - data = spl_fixedarray_object_read_dimension_helper(object, &zindex); - - if (data == NULL) { - data = &EG(uninitialized_zval); - } - return data; + if (data == NULL) { + data = &EG(uninitialized_zval); } + return data; } /* }}} */ static void spl_fixedarray_it_get_current_key(zend_object_iterator *iter, zval *key) /* {{{ */ { - spl_fixedarray_object *object = Z_SPLFIXEDARRAY_P(&iter->data); - - if (object->flags & SPL_FIXEDARRAY_OVERLOADED_KEY) { - zend_user_it_get_current_key(iter, key); - } else { - ZVAL_LONG(key, object->current); - } + ZVAL_LONG(key, ((spl_fixedarray_it*)iter)->current); } /* }}} */ static void spl_fixedarray_it_move_forward(zend_object_iterator *iter) /* {{{ */ { - spl_fixedarray_object *object = Z_SPLFIXEDARRAY_P(&iter->data); - - if (object->flags & SPL_FIXEDARRAY_OVERLOADED_NEXT) { - zend_user_it_move_forward(iter); - } else { - zend_user_it_invalidate_current(iter); - object->current++; - } -} -/* }}} */ - -/* {{{ Return current array key */ -PHP_METHOD(SplFixedArray, key) -{ - spl_fixedarray_object *intern = Z_SPLFIXEDARRAY_P(ZEND_THIS); - - if (zend_parse_parameters_none() == FAILURE) { - RETURN_THROWS(); - } - - RETURN_LONG(intern->current); -} -/* }}} */ - -/* {{{ Move to next entry */ -PHP_METHOD(SplFixedArray, next) -{ - spl_fixedarray_object *intern = Z_SPLFIXEDARRAY_P(ZEND_THIS); - - if (zend_parse_parameters_none() == FAILURE) { - RETURN_THROWS(); - } - - intern->current++; -} -/* }}} */ - -/* {{{ Check whether the datastructure contains more entries */ -PHP_METHOD(SplFixedArray, valid) -{ - spl_fixedarray_object *intern = Z_SPLFIXEDARRAY_P(ZEND_THIS); - - if (zend_parse_parameters_none() == FAILURE) { - RETURN_THROWS(); - } - - RETURN_BOOL(intern->current >= 0 && intern->current < intern->array.size); -} -/* }}} */ - -/* {{{ Rewind the datastructure back to the start */ -PHP_METHOD(SplFixedArray, rewind) -{ - spl_fixedarray_object *intern = Z_SPLFIXEDARRAY_P(ZEND_THIS); - - if (zend_parse_parameters_none() == FAILURE) { - RETURN_THROWS(); - } - - intern->current = 0; -} -/* }}} */ - -/* {{{ Return current datastructure entry */ -PHP_METHOD(SplFixedArray, current) -{ - zval zindex, *value; - spl_fixedarray_object *intern = Z_SPLFIXEDARRAY_P(ZEND_THIS); - - if (zend_parse_parameters_none() == FAILURE) { - RETURN_THROWS(); - } - - ZVAL_LONG(&zindex, intern->current); - - value = spl_fixedarray_object_read_dimension_helper(intern, &zindex); - - if (value) { - ZVAL_COPY_DEREF(return_value, value); - } else { - RETURN_NULL(); - } + ((spl_fixedarray_it*)iter)->current++; } /* }}} */ @@ -963,12 +832,10 @@ zend_object_iterator *spl_fixedarray_get_iterator(zend_class_entry *ce, zval *ob zend_iterator_init((zend_object_iterator*)iterator); - ZVAL_OBJ_COPY(&iterator->intern.it.data, Z_OBJ_P(object)); - iterator->intern.it.funcs = &spl_fixedarray_it_funcs; - iterator->intern.ce = ce; - ZVAL_UNDEF(&iterator->intern.value); + ZVAL_OBJ_COPY(&iterator->intern.data, Z_OBJ_P(object)); + iterator->intern.funcs = &spl_fixedarray_it_funcs; - return &iterator->intern.it; + return &iterator->intern; } /* }}} */ @@ -990,7 +857,7 @@ PHP_MINIT_FUNCTION(spl_fixedarray) spl_handler_SplFixedArray.dtor_obj = zend_objects_destroy_object; spl_handler_SplFixedArray.free_obj = spl_fixedarray_object_free_storage; - REGISTER_SPL_IMPLEMENTS(SplFixedArray, Iterator); + REGISTER_SPL_IMPLEMENTS(SplFixedArray, Aggregate); REGISTER_SPL_IMPLEMENTS(SplFixedArray, ArrayAccess); REGISTER_SPL_IMPLEMENTS(SplFixedArray, Countable); diff --git a/ext/spl/spl_fixedarray.stub.php b/ext/spl/spl_fixedarray.stub.php index 553de5c2faf3..b169dfdeac78 100755 --- a/ext/spl/spl_fixedarray.stub.php +++ b/ext/spl/spl_fixedarray.stub.php @@ -2,7 +2,7 @@ /** @generate-function-entries */ -class SplFixedArray implements Iterator, ArrayAccess, Countable +class SplFixedArray implements IteratorAggregate, ArrayAccess, Countable { public function __construct(int $size = 0) {} @@ -48,18 +48,5 @@ public function offsetSet($index, mixed $value) {} */ public function offsetUnset($index) {} - /** @return void */ - public function rewind() {} - - /** @return mixed */ - public function current() {} - - /** @return int */ - public function key() {} - - /** @return void */ - public function next() {} - - /** @return bool */ - public function valid() {} + public function getIterator(): Iterator {} } diff --git a/ext/spl/spl_fixedarray_arginfo.h b/ext/spl/spl_fixedarray_arginfo.h index 899202683e82..6817380c6bfa 100644 --- a/ext/spl/spl_fixedarray_arginfo.h +++ b/ext/spl/spl_fixedarray_arginfo.h @@ -1,5 +1,5 @@ /* This is a generated file, edit the .stub.php file instead. - * Stub hash: 4b6c37c54416ee46f610baba2a8b2be45d1db96f */ + * Stub hash: 2075619a330ed636ce7f7c85aeb208a8f9f55ca7 */ ZEND_BEGIN_ARG_INFO_EX(arginfo_class_SplFixedArray___construct, 0, 0, 0) ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, size, IS_LONG, 0, "0") @@ -36,15 +36,8 @@ ZEND_END_ARG_INFO() #define arginfo_class_SplFixedArray_offsetUnset arginfo_class_SplFixedArray_offsetExists -#define arginfo_class_SplFixedArray_rewind arginfo_class_SplFixedArray___wakeup - -#define arginfo_class_SplFixedArray_current arginfo_class_SplFixedArray___wakeup - -#define arginfo_class_SplFixedArray_key arginfo_class_SplFixedArray___wakeup - -#define arginfo_class_SplFixedArray_next arginfo_class_SplFixedArray___wakeup - -#define arginfo_class_SplFixedArray_valid arginfo_class_SplFixedArray___wakeup +ZEND_BEGIN_ARG_WITH_RETURN_OBJ_INFO_EX(arginfo_class_SplFixedArray_getIterator, 0, 0, Iterator, 0) +ZEND_END_ARG_INFO() ZEND_METHOD(SplFixedArray, __construct); @@ -58,11 +51,7 @@ ZEND_METHOD(SplFixedArray, offsetExists); ZEND_METHOD(SplFixedArray, offsetGet); ZEND_METHOD(SplFixedArray, offsetSet); ZEND_METHOD(SplFixedArray, offsetUnset); -ZEND_METHOD(SplFixedArray, rewind); -ZEND_METHOD(SplFixedArray, current); -ZEND_METHOD(SplFixedArray, key); -ZEND_METHOD(SplFixedArray, next); -ZEND_METHOD(SplFixedArray, valid); +ZEND_METHOD(SplFixedArray, getIterator); static const zend_function_entry class_SplFixedArray_methods[] = { @@ -77,10 +66,6 @@ static const zend_function_entry class_SplFixedArray_methods[] = { ZEND_ME(SplFixedArray, offsetGet, arginfo_class_SplFixedArray_offsetGet, ZEND_ACC_PUBLIC) ZEND_ME(SplFixedArray, offsetSet, arginfo_class_SplFixedArray_offsetSet, ZEND_ACC_PUBLIC) ZEND_ME(SplFixedArray, offsetUnset, arginfo_class_SplFixedArray_offsetUnset, ZEND_ACC_PUBLIC) - ZEND_ME(SplFixedArray, rewind, arginfo_class_SplFixedArray_rewind, ZEND_ACC_PUBLIC) - ZEND_ME(SplFixedArray, current, arginfo_class_SplFixedArray_current, ZEND_ACC_PUBLIC) - ZEND_ME(SplFixedArray, key, arginfo_class_SplFixedArray_key, ZEND_ACC_PUBLIC) - ZEND_ME(SplFixedArray, next, arginfo_class_SplFixedArray_next, ZEND_ACC_PUBLIC) - ZEND_ME(SplFixedArray, valid, arginfo_class_SplFixedArray_valid, ZEND_ACC_PUBLIC) + ZEND_ME(SplFixedArray, getIterator, arginfo_class_SplFixedArray_getIterator, ZEND_ACC_PUBLIC) ZEND_FE_END }; diff --git a/ext/spl/tests/SplFixedArray_change_size_during_iteration.phpt b/ext/spl/tests/SplFixedArray_change_size_during_iteration.phpt new file mode 100644 index 000000000000..56565ff7aac8 --- /dev/null +++ b/ext/spl/tests/SplFixedArray_change_size_during_iteration.phpt @@ -0,0 +1,43 @@ +--TEST-- +SPL: FixedArray: change array size during iteration +--FILE-- + $v) { + echo "$k => $v\n"; + if ($k == 0) { + $splFixedArray->setSize(2); + } +} +echo "---\n"; + +$splFixedArray = SplFixedArray::fromArray(["a","b","c"]); +foreach ($splFixedArray as $k => $v) { + echo "$k => $v\n"; + if ($k == 1) { + $splFixedArray->setSize(2); + } +} +echo "---\n"; + +$splFixedArray = SplFixedArray::fromArray(["a","b","c"]); +foreach ($splFixedArray as $k => $v) { + echo "$k => $v\n"; + if ($k == 2) { + $splFixedArray->setSize(2); + } +} +echo "\n"; +--EXPECT-- +0 => a +1 => b +--- +0 => a +1 => b +--- +0 => a +1 => b +2 => c diff --git a/ext/spl/tests/SplFixedArray_key_setsize.phpt b/ext/spl/tests/SplFixedArray_key_setsize.phpt deleted file mode 100644 index 5c035e71c777..000000000000 --- a/ext/spl/tests/SplFixedArray_key_setsize.phpt +++ /dev/null @@ -1,20 +0,0 @@ ---TEST-- -SplFixedArray::key() when the array has a size higher than the amount of values specified. ---CREDITS-- -PHPNW Test Fest 2009 - Jordan Hatch ---FILE-- -key( ); -} - -?> ---EXPECT-- -0123 diff --git a/ext/spl/tests/SplFixedArray_nested_foreach.phpt b/ext/spl/tests/SplFixedArray_nested_foreach.phpt new file mode 100644 index 000000000000..0042ab789052 --- /dev/null +++ b/ext/spl/tests/SplFixedArray_nested_foreach.phpt @@ -0,0 +1,19 @@ +--TEST-- +Nested iteration of SplFixedArray using foreach loops +--FILE-- + +--EXPECT-- +0 0 +0 1 +1 0 +1 1 diff --git a/ext/spl/tests/SplFixedArray_override_getIterator.phpt b/ext/spl/tests/SplFixedArray_override_getIterator.phpt new file mode 100644 index 000000000000..52bf52f2a1b5 --- /dev/null +++ b/ext/spl/tests/SplFixedArray_override_getIterator.phpt @@ -0,0 +1,46 @@ +--TEST-- +SPL: FixedArray: overriding getIterator() +--FILE-- +valid()) { + echo "In A: key={$iterator->key()} value={$iterator->current()}\n"; + yield $iterator->key() => $iterator->current(); + $iterator->next(); + } + } +} + +echo "==SplFixedArray instance==\n"; +$a = new SplFixedArray(3); +$a[0] = "a"; +$a[1] = "b"; +$a[2] = "c"; +foreach ($a as $k => $v) { + echo "$k => $v\n"; +} + +echo "==Subclass instance==\n"; +$a = new A(3); +$a[0] = "d"; +$a[1] = "e"; +$a[2] = "f"; +foreach ($a as $k => $v) { + echo "$k => $v\n"; +} +--EXPECT-- +==SplFixedArray instance== +0 => a +1 => b +2 => c +==Subclass instance== +In A: key=0 value=d +0 => d +In A: key=1 value=e +1 => e +In A: key=2 value=f +2 => f diff --git a/ext/spl/tests/fixedarray_003.phpt b/ext/spl/tests/fixedarray_003.phpt deleted file mode 100644 index d2f8a78fdf08..000000000000 --- a/ext/spl/tests/fixedarray_003.phpt +++ /dev/null @@ -1,86 +0,0 @@ ---TEST-- -SPL: FixedArray: Iterators ---FILE-- - $v) { - echo "$k => $v\n"; -} -echo "==Child instance==\n"; -$a = new A(5); -$a[0] = "a"; -$a[1] = "c"; -$a[2] = "d"; -$a[3] = "e"; -$a[4] = "f"; -foreach ($a as $k => $v) { - echo "$k => $v\n"; -} -?> ---EXPECT-- -==Direct instance== -0 => a -1 => c -2 => d -3 => e -4 => f -==Child instance== -A::rewind -A::valid -A::current -A::key -0 => a -A::next -A::valid -A::current -A::key -1 => c -A::next -A::valid -A::current -A::key -2 => d -A::next -A::valid -A::current -A::key -3 => e -A::next -A::valid -A::current -A::key -4 => f -A::next -A::valid diff --git a/ext/spl/tests/fixedarray_019.phpt b/ext/spl/tests/fixedarray_019.phpt deleted file mode 100644 index 9414e9730450..000000000000 --- a/ext/spl/tests/fixedarray_019.phpt +++ /dev/null @@ -1,51 +0,0 @@ ---TEST-- -SPL: FixedArray: overridden iterator methods ---FILE-- -$v) { - echo "$k=>"; - var_dump($v); -} -?> ---EXPECT-- -rewind -valid -current -key -0=>NULL -next -valid -current -key -1=>NULL -next -valid -current -key -2=>NULL -next -valid