Skip to content

Commit

Permalink
SplFixedArray is Aggregate, not Iterable
Browse files Browse the repository at this point in the history 
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!
  • Loading branch information
@alexdowad
alexdowad committed Sep 23, 2020
1 parent 1e9db80 commit 4222ae1
Show file tree
Hide file tree
Showing 11 changed files with 162 additions and 364 deletions.
2 changes: 2 additions & 0 deletions NEWS
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ PHP NEWS
- Core:
. Fixed bug #80109 (Cannot skip arguments when extended debug is enabled).
(Nikita)
- SPL:
. SplFixedArray is now IteratorAggregate rather than Iterator. (alexdowad)

17 Sep 2020, PHP 8.0.0beta4

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

- Standard:
. assert() will no longer evaluate string arguments, instead they will be
Expand Down
211 changes: 39 additions & 172 deletions ext/spl/spl_fixedarray.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
Expand Down Expand Up @@ -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);
Expand All @@ -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;
Expand Down Expand Up @@ -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;
}

Expand All @@ -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++;
}
/* }}} */

Expand Down Expand Up @@ -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;
}
/* }}} */

Expand All @@ -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);

Expand Down
17 changes: 2 additions & 15 deletions ext/spl/spl_fixedarray.stub.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {}

Expand Down Expand Up @@ -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 {}
}
Loading

0 comments on commit 4222ae1

Please sign in to comment.