Skip to content

Spl object storage refactor #13666

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
+35 −29
Closed

Spl object storage refactor #13666

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b4c2cd8
ext/spl: Move MultipleIterator tests to its own folder
Girgias Mar 17, 2024
6945677
ext/spl: don't loop unnecessarily when creating SplObjectStorage
Girgias Mar 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 34 additions & 28 deletions ext/spl/spl_observer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,9 +255,8 @@ static void spl_object_storage_addall(spl_SplObjectStorage *intern, spl_SplObjec
static zend_object *spl_object_storage_new_ex(zend_class_entry *class_type, zend_object *orig) /* {{{ */
{
spl_SplObjectStorage *intern;
zend_class_entry *parent = class_type;

intern = emalloc(sizeof(spl_SplObjectStorage) + zend_object_properties_size(parent));
intern = emalloc(sizeof(spl_SplObjectStorage) + zend_object_properties_size(class_type));
memset(intern, 0, sizeof(spl_SplObjectStorage) - sizeof(zval));
intern->pos = 0;

Expand All @@ -266,37 +265,44 @@ static zend_object *spl_object_storage_new_ex(zend_class_entry *class_type, zend

zend_hash_init(&intern->storage, 0, NULL, spl_object_storage_dtor, 0);

while (parent) {
if (parent == spl_ce_SplObjectStorage) {
/* Possible optimization: Cache these results with a map from class entry to IS_NULL/IS_PTR.
* Or maybe just a single item with the result for the most recently loaded subclass. */
if (class_type != spl_ce_SplObjectStorage) {
zend_function *get_hash = zend_hash_str_find_ptr(&class_type->function_table, "gethash", sizeof("gethash") - 1);
if (get_hash->common.scope != spl_ce_SplObjectStorage) {
intern->fptr_get_hash = get_hash;
}
if (intern->fptr_get_hash != NULL ||
SPL_OBJECT_STORAGE_CLASS_HAS_OVERRIDE(class_type, zf_offsetget) ||
SPL_OBJECT_STORAGE_CLASS_HAS_OVERRIDE(class_type, zf_offsetexists)) {
intern->flags |= SOS_OVERRIDDEN_READ_DIMENSION;
}

if (intern->fptr_get_hash != NULL ||
SPL_OBJECT_STORAGE_CLASS_HAS_OVERRIDE(class_type, zf_offsetset)) {
intern->flags |= SOS_OVERRIDDEN_WRITE_DIMENSION;
}

if (intern->fptr_get_hash != NULL ||
SPL_OBJECT_STORAGE_CLASS_HAS_OVERRIDE(class_type, zf_offsetunset)) {
intern->flags |= SOS_OVERRIDDEN_UNSET_DIMENSION;
}
/* Child classes may overload internal access methods */
if (class_type != spl_ce_SplObjectStorage) {
zend_class_entry *parent = class_type;
do {
/* TODO: MultipleIterator uses SplObjectStorage, which makes no sense
* need to convert it to just use a normal backing HashTable
* in the meantime we skip this whole block if it is this case. */
if (parent == spl_ce_MultipleIterator) {
goto end;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's now harder to follow the logic, I'm not sure this is an improvement.
If you fix this TODO and this code in a follow up then it's fine I think.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am planning to, but as I'm going on holiday I will leave this PR open so that I dont' forget about it.

}
break;
parent = parent->parent;
} while (parent != spl_ce_SplObjectStorage);

ZEND_ASSERT(parent == spl_ce_SplObjectStorage);
/* Possible optimization: Cache these results with a map from class entry to IS_NULL/IS_PTR.
* Or maybe just a single item with the result for the most recently loaded subclass. */
zend_function *get_hash = zend_hash_str_find_ptr(&class_type->function_table, "gethash", sizeof("gethash") - 1);
if (get_hash->common.scope != spl_ce_SplObjectStorage) {
intern->fptr_get_hash = get_hash;
}
if (intern->fptr_get_hash != NULL ||
SPL_OBJECT_STORAGE_CLASS_HAS_OVERRIDE(class_type, zf_offsetget) ||
SPL_OBJECT_STORAGE_CLASS_HAS_OVERRIDE(class_type, zf_offsetexists)) {
intern->flags |= SOS_OVERRIDDEN_READ_DIMENSION;
}

if (intern->fptr_get_hash != NULL ||
SPL_OBJECT_STORAGE_CLASS_HAS_OVERRIDE(class_type, zf_offsetset)) {
intern->flags |= SOS_OVERRIDDEN_WRITE_DIMENSION;
}

parent = parent->parent;
if (intern->fptr_get_hash != NULL ||
SPL_OBJECT_STORAGE_CLASS_HAS_OVERRIDE(class_type, zf_offsetunset)) {
intern->flags |= SOS_OVERRIDDEN_UNSET_DIMENSION;
}
}

end:
if (orig) {
spl_SplObjectStorage *other = spl_object_storage_from_obj(orig);
spl_object_storage_addall(intern, other);
Expand Down
2 changes: 1 addition & 1 deletion ext/spl/tests/bug70053.phpt → ext/spl/tests/MultipleIterator/bug70053.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
--TEST--
SPL: ArrayObject
Bug #70053 (MutlitpleIterator array-keys incompatible change in PHP 7)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in MultipleIterator

--FILE--
<?php

Expand Down
0 ext/spl/tests/bug81587.phpt → ext/spl/tests/MultipleIterator/bug81587.phpt
View file
Open in desktop
File renamed without changes.
0 ext/spl/tests/multiple_iterator_001.phpt → ...ltipleIterator/multiple_iterator_001.phpt
View file
Open in desktop
File renamed without changes.