Skip to content
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.

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

ext/spl: Various refactorings #14518

Merged
merged 10 commits into from
Jun 15, 2024
Merged

ext/spl: Various refactorings #14518

merged 10 commits into from
Jun 15, 2024

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Jun 9, 2024

No description provided.

@Girgias Girgias marked this pull request as ready for review June 9, 2024 04:06
@Girgias Girgias requested a review from nielsdos June 9, 2024 04:08
ext/spl/spl_directory.c Show resolved Hide resolved
ext/spl/spl_directory.c Outdated Show resolved Hide resolved
zend_symtable_update(debug_info, zname, storage);
zend_string_release_ex(zname, 0);

zend_string *mangled_named = zend_mangle_property_name(
Copy link
Member

Choose a reason for hiding this comment

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

IMO there was nothing wrong with the old code having a specialised helper, now there's more repetition in the code. I would prefer this commit to be dropped.

Copy link
Member Author

Choose a reason for hiding this comment

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

In that case I would prefer an API that actually sets the hashtable value at the same time, rather than just get the mangled name.

I'll see what I can cook up.

@@ -783,11 +780,18 @@ static inline HashTable* spl_array_get_debug_info(zend_object *obj) /* {{{ */
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

ext/spl/spl_directory.c Outdated Show resolved Hide resolved
Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

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

Looks much better

ZVAL_ARR(&intern->array,
zend_array_dup(spl_array_get_hash_table(other)));
} else {
ZEND_ASSERT(orig->handlers == &spl_handler_ArrayIterator);
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.

@Girgias
Copy link
Member Author

Girgias commented Jun 14, 2024

I rearranged the commits and folded the relevant fixups into the appropriate commits

@Girgias Girgias merged commit f0fb9e3 into php:master Jun 15, 2024
10 of 11 checks passed
@Girgias Girgias deleted the spl-refactoring branch June 15, 2024 00:33
memset(intern, 0,
MAX(XtOffsetOf(spl_filesystem_object, u.dir.entry),
XtOffsetOf(spl_filesystem_object, u.file.escape) + sizeof(int)));
intern = ecalloc(1, sizeof(spl_filesystem_object) + zend_object_properties_size(class_type));
Copy link
Member

Choose a reason for hiding this comment

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

I stumbled on this by accident. The memset was intentional to avoid writing the entirety of spl_filesystem_object.u.dir.entry. Did this cause any issues? If not, I think this should be reverted.

Copy link
Member Author

Choose a reason for hiding this comment

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

Doesn't ecalloc take care of setting the whole memory to 0 tho?

Copy link
Member

@iluuu1994 iluuu1994 Oct 17, 2024

Choose a reason for hiding this comment

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

Yes, but we don't need to do that. spl_filesystem_object.u.dir.entry will later be initialized, and we don't need to initialize the entire structure anyway.

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 right, I personally find the memset confusing, which is why I replaced it with a calloc. I don't really care but then SPL is convoluted enough that being clear seems like a positive to me.

Copy link
Member

Choose a reason for hiding this comment

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

A comment should suffice :) I'll take care of it. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants