Skip to content

ext/spl: Various refactorings #14518

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

Merged
merged 10 commits into from
Jun 15, 2024
Merged

ext/spl: Various refactorings #14518

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
58d5cf5
ext/spl: Refactor SplFileInfo::getPathInfo() implementation
Girgias Jun 8, 2024
2100ce3
ext/spl: Remove useless wrapper
Girgias Jun 9, 2024
bad96aa
ext/spl: Adding a const modifier
Girgias Jun 9, 2024
7b3c99e
ext/spl: Convert current_line to a zend_string*
Girgias Jun 9, 2024
120c685
ext/spl: Refactor SplFileObject::fgetc()
Girgias Jun 9, 2024
70779c0
ext/spl: Remove some useless header includes and clarify usages
Girgias Jun 9, 2024
2dc8677
ext/spl: Refactor debug handlers
Girgias Jun 9, 2024
4417f0f
ext/spl: Remove unused typedef
Girgias Jun 9, 2024
a1736a0
ext/spl: Use ArrayObject object handlers for ArrayIterator
Girgias Jun 9, 2024
5463c2d
ext/spl: use ecalloc instead of complicate memset to 0
Girgias Jun 9, 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
33 changes: 16 additions & 17 deletions ext/spl/spl_array.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,13 @@
#include "zend_interfaces.h"
#include "zend_exceptions.h"

#include "php_spl.h"
#include "spl_functions.h"
#include "spl_iterators.h"
#include "spl_array.h"
#include "spl_array_arginfo.h"
#include "spl_exceptions.h"
#include "spl_functions.h" /* For spl_set_private_debug_info_property() */

/* Defined later in the file */
static zend_object_handlers spl_handler_ArrayIterator;
PHPAPI zend_class_entry *spl_ce_ArrayIterator;
PHPAPI zend_class_entry *spl_ce_RecursiveArrayIterator;

Expand Down Expand Up @@ -163,11 +161,18 @@ static zend_object *spl_array_object_new_ex(zend_class_entry *class_type, zend_o
if (clone_orig) {
if (other->ar_flags & SPL_ARRAY_IS_SELF) {
ZVAL_UNDEF(&intern->array);
} else if (orig->handlers == &spl_handler_ArrayObject) {
} else if (instanceof_function(class_type, spl_ce_ArrayObject)) {
ZVAL_ARR(&intern->array,
zend_array_dup(spl_array_get_hash_table(other)));
} else {
ZEND_ASSERT(orig->handlers == &spl_handler_ArrayIterator);
#if ZEND_DEBUG
/* This is because the call to instanceof_function will remain because
* the compiler can't prove in this compile unit that this function is
* side-effect-free.
* See https://github.com/php/php-src/pull/14518#discussion_r1638740932 */
ZEND_ASSERT(instanceof_function(class_type, spl_ce_ArrayIterator));
Copy link
Member

Choose a reason for hiding this comment

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

Even though asserts are gone in release builds, the call to instanceof_function will remain because the compiler can't prove in this compile unit that this function is side-effect-free. It's a bit unfortunate.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, this is unfortunate indeed, would adding __attribute__((const)) (GCC) attributes to instanceof_function and instanceof_function_slow fix this issue?

Copy link
Member

Choose a reason for hiding this comment

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

Probably.
Or surround the assertion line with #if ZEND_DEBUG #endif.

#endif

ZVAL_OBJ_COPY(&intern->array, orig);
intern->ar_flags |= SPL_ARRAY_USE_OTHER;
}
Expand Down Expand Up @@ -764,9 +769,6 @@ static HashTable *spl_array_get_properties_for(zend_object *object, zend_prop_pu

static inline HashTable* spl_array_get_debug_info(zend_object *obj) /* {{{ */
{
zval *storage;
zend_string *zname;
zend_class_entry *base;
spl_array_object *intern = spl_array_from_obj(obj);

if (!intern->std.properties) {
Expand All @@ -781,14 +783,13 @@ static inline HashTable* spl_array_get_debug_info(zend_object *obj) /* {{{ */
debug_info = zend_new_array(zend_hash_num_elements(intern->std.properties) + 1);
zend_hash_copy(debug_info, intern->std.properties, (copy_ctor_func_t) zval_add_ref);

storage = &intern->array;
zval *storage = &intern->array;
Z_TRY_ADDREF_P(storage);

base = obj->handlers == &spl_handler_ArrayIterator
const zend_class_entry *base_class_ce = instanceof_function(obj->ce, spl_ce_ArrayIterator)
Copy link
Member

Choose a reason for hiding this comment

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

Given SPL's funkyness I'm not so sure changing this is a good idea, this call is also more expensive btw than the previous code.

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 was going to remove the dedicated object handlers as they are identical to the ArrayObject ones, I'd also expect the base case to be fast as subclassing should be relatively rare

Copy link
Member Author

Choose a reason for hiding this comment

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

Another reason is that I'd like to move the ArrayIterator into the iterator.c file in the long run

Copy link
Member

Choose a reason for hiding this comment

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

Okay fair enough

? spl_ce_ArrayIterator : spl_ce_ArrayObject;
zname = spl_gen_private_prop_name(base, "storage", sizeof("storage")-1);
zend_symtable_update(debug_info, zname, storage);
zend_string_release_ex(zname, 0);

spl_set_private_debug_info_property(base_class_ce, "storage", strlen("storage"), debug_info, storage);

return debug_info;
}
Expand Down Expand Up @@ -952,7 +953,7 @@ static void spl_array_set_array(zval *object, spl_array_object *intern, zval *ar
}
}
} else {
if (Z_OBJ_HT_P(array) == &spl_handler_ArrayObject || Z_OBJ_HT_P(array) == &spl_handler_ArrayIterator) {
if (Z_OBJ_HT_P(array) == &spl_handler_ArrayObject) {
zval_ptr_dtor(&intern->array);
if (just_array) {
spl_array_object *other = Z_SPLARRAY_P(array);
Expand Down Expand Up @@ -1900,11 +1901,9 @@ PHP_MINIT_FUNCTION(spl_array)

spl_ce_ArrayIterator = register_class_ArrayIterator(spl_ce_SeekableIterator, zend_ce_arrayaccess, zend_ce_serializable, zend_ce_countable);
spl_ce_ArrayIterator->create_object = spl_array_object_new;
spl_ce_ArrayIterator->default_object_handlers = &spl_handler_ArrayIterator;
spl_ce_ArrayIterator->default_object_handlers = &spl_handler_ArrayObject;
spl_ce_ArrayIterator->get_iterator = spl_array_get_iterator;

memcpy(&spl_handler_ArrayIterator, &spl_handler_ArrayObject, sizeof(zend_object_handlers));

spl_ce_RecursiveArrayIterator = register_class_RecursiveArrayIterator(spl_ce_ArrayIterator, spl_ce_RecursiveIterator);
spl_ce_RecursiveArrayIterator->create_object = spl_array_object_new;
spl_ce_RecursiveArrayIterator->get_iterator = spl_array_get_iterator;
Expand Down
2 changes: 0 additions & 2 deletions ext/spl/spl_array.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@
#define SPL_ARRAY_H

#include "php.h"
#include "php_spl.h"
#include "spl_iterators.h"

#define SPL_ARRAY_STD_PROP_LIST 0x00000001
#define SPL_ARRAY_ARRAY_AS_PROPS 0x00000002
Expand Down
Loading