Skip to content

Commit 4222ae1

Browse files
committed
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 ff19ec2 to convert SplFixedArray to an Aggregate rather than Iterable. As a bonus, we get to trim down some ugly code! Yay!
1 parent 1e9db80 commit 4222ae1

11 files changed

+162
-364
lines changed

NEWS

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@ PHP NEWS
77
- Core:
88
. Fixed bug #80109 (Cannot skip arguments when extended debug is enabled).
99
(Nikita)
10+
- SPL:
11+
. SplFixedArray is now IteratorAggregate rather than Iterator. (alexdowad)
1012

1113
17 Sep 2020, PHP 8.0.0beta4
1214

UPGRADING

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -502,6 +502,12 @@ PHP 8.0 UPGRADE NOTES
502502
. spl_autoload_register() will now always throw a TypeError on invalid
503503
arguments, therefore the second argument $do_throw is ignored and a
504504
notice will be emitted if it is set to false.
505+
. SplFixedArray is now an IteratorAggregate and not an Iterator.
506+
SplFixedArray::rewind(), ::current(), ::key(), ::next(), and ::valid()
507+
have been removed. In their place, SplFixedArray::getIterator() has been
508+
added. Any code which uses explicit iteration over SplFixedArray must now
509+
obtain an Iterator through SplFixedArray::getIterator(). This means that
510+
SplFixedArray is now safe to use in nested loops.
505511

506512
- Standard:
507513
. assert() will no longer evaluate string arguments, instead they will be

ext/spl/spl_fixedarray.c

Lines changed: 39 additions & 172 deletions
Original file line numberDiff line numberDiff line change
@@ -46,29 +46,22 @@ typedef struct _spl_fixedarray { /* {{{ */
4646
/* }}} */
4747

4848
typedef struct _spl_fixedarray_object { /* {{{ */
49-
spl_fixedarray array;
50-
zend_function *fptr_offset_get;
51-
zend_function *fptr_offset_set;
52-
zend_function *fptr_offset_has;
53-
zend_function *fptr_offset_del;
54-
zend_function *fptr_count;
55-
int current;
56-
int flags;
57-
zend_object std;
49+
spl_fixedarray array;
50+
zend_function *fptr_offset_get;
51+
zend_function *fptr_offset_set;
52+
zend_function *fptr_offset_has;
53+
zend_function *fptr_offset_del;
54+
zend_function *fptr_count;
55+
zend_object std;
5856
} spl_fixedarray_object;
5957
/* }}} */
6058

6159
typedef struct _spl_fixedarray_it { /* {{{ */
62-
zend_user_iterator intern;
60+
zend_object_iterator intern;
61+
int current;
6362
} spl_fixedarray_it;
6463
/* }}} */
6564

66-
#define SPL_FIXEDARRAY_OVERLOADED_REWIND 0x0001
67-
#define SPL_FIXEDARRAY_OVERLOADED_VALID 0x0002
68-
#define SPL_FIXEDARRAY_OVERLOADED_KEY 0x0004
69-
#define SPL_FIXEDARRAY_OVERLOADED_CURRENT 0x0008
70-
#define SPL_FIXEDARRAY_OVERLOADED_NEXT 0x0010
71-
7265
static inline spl_fixedarray_object *spl_fixed_array_from_obj(zend_object *obj) /* {{{ */ {
7366
return (spl_fixedarray_object*)((char*)(obj) - XtOffsetOf(spl_fixedarray_object, std));
7467
}
@@ -201,23 +194,17 @@ static void spl_fixedarray_object_free_storage(zend_object *object) /* {{{ */
201194
}
202195
/* }}} */
203196

204-
zend_object_iterator *spl_fixedarray_get_iterator(zend_class_entry *ce, zval *object, int by_ref);
205-
206197
static zend_object *spl_fixedarray_object_new_ex(zend_class_entry *class_type, zend_object *orig, int clone_orig) /* {{{ */
207198
{
208199
spl_fixedarray_object *intern;
209200
zend_class_entry *parent = class_type;
210201
int inherited = 0;
211-
zend_class_iterator_funcs *funcs_ptr;
212202

213203
intern = zend_object_alloc(sizeof(spl_fixedarray_object), parent);
214204

215205
zend_object_std_init(&intern->std, class_type);
216206
object_properties_init(&intern->std, class_type);
217207

218-
intern->current = 0;
219-
intern->flags = 0;
220-
221208
if (orig && clone_orig) {
222209
spl_fixedarray_object *other = spl_fixed_array_from_obj(orig);
223210
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
236223

237224
ZEND_ASSERT(parent);
238225

239-
funcs_ptr = class_type->iterator_funcs_ptr;
240-
if (!funcs_ptr->zf_current) {
241-
funcs_ptr->zf_rewind = zend_hash_str_find_ptr(&class_type->function_table, "rewind", sizeof("rewind") - 1);
242-
funcs_ptr->zf_valid = zend_hash_str_find_ptr(&class_type->function_table, "valid", sizeof("valid") - 1);
243-
funcs_ptr->zf_key = zend_hash_str_find_ptr(&class_type->function_table, "key", sizeof("key") - 1);
244-
funcs_ptr->zf_current = zend_hash_str_find_ptr(&class_type->function_table, "current", sizeof("current") - 1);
245-
funcs_ptr->zf_next = zend_hash_str_find_ptr(&class_type->function_table, "next", sizeof("next") - 1);
246-
}
247226
if (inherited) {
248-
if (funcs_ptr->zf_rewind->common.scope != parent) {
249-
intern->flags |= SPL_FIXEDARRAY_OVERLOADED_REWIND;
250-
}
251-
if (funcs_ptr->zf_valid->common.scope != parent) {
252-
intern->flags |= SPL_FIXEDARRAY_OVERLOADED_VALID;
253-
}
254-
if (funcs_ptr->zf_key->common.scope != parent) {
255-
intern->flags |= SPL_FIXEDARRAY_OVERLOADED_KEY;
256-
}
257-
if (funcs_ptr->zf_current->common.scope != parent) {
258-
intern->flags |= SPL_FIXEDARRAY_OVERLOADED_CURRENT;
259-
}
260-
if (funcs_ptr->zf_next->common.scope != parent) {
261-
intern->flags |= SPL_FIXEDARRAY_OVERLOADED_NEXT;
262-
}
263-
264227
intern->fptr_offset_get = zend_hash_str_find_ptr(&class_type->function_table, "offsetget", sizeof("offsetget") - 1);
265228
if (intern->fptr_offset_get->common.scope == parent) {
266229
intern->fptr_offset_get = NULL;
@@ -780,36 +743,35 @@ PHP_METHOD(SplFixedArray, offsetUnset)
780743

781744
} /* }}} */
782745

783-
static void spl_fixedarray_it_dtor(zend_object_iterator *iter) /* {{{ */
746+
/* {{{ Create a new iterator from a SplFixedArray instance. */
747+
PHP_METHOD(SplFixedArray, getIterator)
784748
{
785-
spl_fixedarray_it *iterator = (spl_fixedarray_it *)iter;
749+
if (zend_parse_parameters_none() == FAILURE) {
750+
return;
751+
}
786752

787-
zend_user_it_invalidate_current(iter);
788-
zval_ptr_dtor(&iterator->intern.it.data);
753+
zend_create_internal_iterator_zval(return_value, ZEND_THIS);
789754
}
790755
/* }}} */
791756

792-
static void spl_fixedarray_it_rewind(zend_object_iterator *iter) /* {{{ */
757+
static void spl_fixedarray_it_dtor(zend_object_iterator *iter) /* {{{ */
793758
{
794-
spl_fixedarray_object *object = Z_SPLFIXEDARRAY_P(&iter->data);
759+
zval_ptr_dtor(&iter->data);
760+
}
761+
/* }}} */
795762

796-
if (object->flags & SPL_FIXEDARRAY_OVERLOADED_REWIND) {
797-
zend_user_it_rewind(iter);
798-
} else {
799-
object->current = 0;
800-
}
763+
static void spl_fixedarray_it_rewind(zend_object_iterator *iter) /* {{{ */
764+
{
765+
((spl_fixedarray_it*)iter)->current = 0;
801766
}
802767
/* }}} */
803768

804769
static int spl_fixedarray_it_valid(zend_object_iterator *iter) /* {{{ */
805770
{
806-
spl_fixedarray_object *object = Z_SPLFIXEDARRAY_P(&iter->data);
771+
spl_fixedarray_it *iterator = (spl_fixedarray_it*)iter;
772+
spl_fixedarray_object *object = Z_SPLFIXEDARRAY_P(&iter->data);
807773

808-
if (object->flags & SPL_FIXEDARRAY_OVERLOADED_VALID) {
809-
return zend_user_it_valid(iter);
810-
}
811-
812-
if (object->current >= 0 && object->current < object->array.size) {
774+
if (iterator->current >= 0 && iterator->current < object->array.size) {
813775
return SUCCESS;
814776
}
815777

@@ -819,122 +781,29 @@ static int spl_fixedarray_it_valid(zend_object_iterator *iter) /* {{{ */
819781

820782
static zval *spl_fixedarray_it_get_current_data(zend_object_iterator *iter) /* {{{ */
821783
{
822-
zval zindex;
823-
spl_fixedarray_object *object = Z_SPLFIXEDARRAY_P(&iter->data);
824-
825-
if (object->flags & SPL_FIXEDARRAY_OVERLOADED_CURRENT) {
826-
return zend_user_it_get_current_data(iter);
827-
} else {
828-
zval *data;
784+
zval zindex, *data;
785+
spl_fixedarray_it *iterator = (spl_fixedarray_it*)iter;
786+
spl_fixedarray_object *object = Z_SPLFIXEDARRAY_P(&iter->data);
829787

830-
ZVAL_LONG(&zindex, object->current);
788+
ZVAL_LONG(&zindex, iterator->current);
789+
data = spl_fixedarray_object_read_dimension_helper(object, &zindex);
831790

832-
data = spl_fixedarray_object_read_dimension_helper(object, &zindex);
833-
834-
if (data == NULL) {
835-
data = &EG(uninitialized_zval);
836-
}
837-
return data;
791+
if (data == NULL) {
792+
data = &EG(uninitialized_zval);
838793
}
794+
return data;
839795
}
840796
/* }}} */
841797

842798
static void spl_fixedarray_it_get_current_key(zend_object_iterator *iter, zval *key) /* {{{ */
843799
{
844-
spl_fixedarray_object *object = Z_SPLFIXEDARRAY_P(&iter->data);
845-
846-
if (object->flags & SPL_FIXEDARRAY_OVERLOADED_KEY) {
847-
zend_user_it_get_current_key(iter, key);
848-
} else {
849-
ZVAL_LONG(key, object->current);
850-
}
800+
ZVAL_LONG(key, ((spl_fixedarray_it*)iter)->current);
851801
}
852802
/* }}} */
853803

854804
static void spl_fixedarray_it_move_forward(zend_object_iterator *iter) /* {{{ */
855805
{
856-
spl_fixedarray_object *object = Z_SPLFIXEDARRAY_P(&iter->data);
857-
858-
if (object->flags & SPL_FIXEDARRAY_OVERLOADED_NEXT) {
859-
zend_user_it_move_forward(iter);
860-
} else {
861-
zend_user_it_invalidate_current(iter);
862-
object->current++;
863-
}
864-
}
865-
/* }}} */
866-
867-
/* {{{ Return current array key */
868-
PHP_METHOD(SplFixedArray, key)
869-
{
870-
spl_fixedarray_object *intern = Z_SPLFIXEDARRAY_P(ZEND_THIS);
871-
872-
if (zend_parse_parameters_none() == FAILURE) {
873-
RETURN_THROWS();
874-
}
875-
876-
RETURN_LONG(intern->current);
877-
}
878-
/* }}} */
879-
880-
/* {{{ Move to next entry */
881-
PHP_METHOD(SplFixedArray, next)
882-
{
883-
spl_fixedarray_object *intern = Z_SPLFIXEDARRAY_P(ZEND_THIS);
884-
885-
if (zend_parse_parameters_none() == FAILURE) {
886-
RETURN_THROWS();
887-
}
888-
889-
intern->current++;
890-
}
891-
/* }}} */
892-
893-
/* {{{ Check whether the datastructure contains more entries */
894-
PHP_METHOD(SplFixedArray, valid)
895-
{
896-
spl_fixedarray_object *intern = Z_SPLFIXEDARRAY_P(ZEND_THIS);
897-
898-
if (zend_parse_parameters_none() == FAILURE) {
899-
RETURN_THROWS();
900-
}
901-
902-
RETURN_BOOL(intern->current >= 0 && intern->current < intern->array.size);
903-
}
904-
/* }}} */
905-
906-
/* {{{ Rewind the datastructure back to the start */
907-
PHP_METHOD(SplFixedArray, rewind)
908-
{
909-
spl_fixedarray_object *intern = Z_SPLFIXEDARRAY_P(ZEND_THIS);
910-
911-
if (zend_parse_parameters_none() == FAILURE) {
912-
RETURN_THROWS();
913-
}
914-
915-
intern->current = 0;
916-
}
917-
/* }}} */
918-
919-
/* {{{ Return current datastructure entry */
920-
PHP_METHOD(SplFixedArray, current)
921-
{
922-
zval zindex, *value;
923-
spl_fixedarray_object *intern = Z_SPLFIXEDARRAY_P(ZEND_THIS);
924-
925-
if (zend_parse_parameters_none() == FAILURE) {
926-
RETURN_THROWS();
927-
}
928-
929-
ZVAL_LONG(&zindex, intern->current);
930-
931-
value = spl_fixedarray_object_read_dimension_helper(intern, &zindex);
932-
933-
if (value) {
934-
ZVAL_COPY_DEREF(return_value, value);
935-
} else {
936-
RETURN_NULL();
937-
}
806+
((spl_fixedarray_it*)iter)->current++;
938807
}
939808
/* }}} */
940809

@@ -963,12 +832,10 @@ zend_object_iterator *spl_fixedarray_get_iterator(zend_class_entry *ce, zval *ob
963832

964833
zend_iterator_init((zend_object_iterator*)iterator);
965834

966-
ZVAL_OBJ_COPY(&iterator->intern.it.data, Z_OBJ_P(object));
967-
iterator->intern.it.funcs = &spl_fixedarray_it_funcs;
968-
iterator->intern.ce = ce;
969-
ZVAL_UNDEF(&iterator->intern.value);
835+
ZVAL_OBJ_COPY(&iterator->intern.data, Z_OBJ_P(object));
836+
iterator->intern.funcs = &spl_fixedarray_it_funcs;
970837

971-
return &iterator->intern.it;
838+
return &iterator->intern;
972839
}
973840
/* }}} */
974841

@@ -990,7 +857,7 @@ PHP_MINIT_FUNCTION(spl_fixedarray)
990857
spl_handler_SplFixedArray.dtor_obj = zend_objects_destroy_object;
991858
spl_handler_SplFixedArray.free_obj = spl_fixedarray_object_free_storage;
992859

993-
REGISTER_SPL_IMPLEMENTS(SplFixedArray, Iterator);
860+
REGISTER_SPL_IMPLEMENTS(SplFixedArray, Aggregate);
994861
REGISTER_SPL_IMPLEMENTS(SplFixedArray, ArrayAccess);
995862
REGISTER_SPL_IMPLEMENTS(SplFixedArray, Countable);
996863

ext/spl/spl_fixedarray.stub.php

Lines changed: 2 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22

33
/** @generate-function-entries */
44

5-
class SplFixedArray implements Iterator, ArrayAccess, Countable
5+
class SplFixedArray implements IteratorAggregate, ArrayAccess, Countable
66
{
77
public function __construct(int $size = 0) {}
88

@@ -48,18 +48,5 @@ public function offsetSet($index, mixed $value) {}
4848
*/
4949
public function offsetUnset($index) {}
5050

51-
/** @return void */
52-
public function rewind() {}
53-
54-
/** @return mixed */
55-
public function current() {}
56-
57-
/** @return int */
58-
public function key() {}
59-
60-
/** @return void */
61-
public function next() {}
62-
63-
/** @return bool */
64-
public function valid() {}
51+
public function getIterator(): Iterator {}
6552
}

0 commit comments

Comments
 (0)