Skip to content

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Oct 27, 2022

This PR improves the FCC struct and API and contains a use case of the improved API.

First, a closure field is added to the FCC struct to keep a reference for closure when the callable is an object.

Secondly, we add APIs to add/del FCC references to manage userland callables references in a global API. We also add some QoL APIs to call FCCs, compare them, and check that they are initialized.

Thirdly, we use this new API in a concrete refactoring case in SPL.

Copy link
Member

@arnaud-lb arnaud-lb left a comment

Choose a reason for hiding this comment

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

This seems sensible, but I would try to use the new APIs in a few additional use-cases, to validate them

Comment on lines 2169 to 2174
if (object->u.callback_filter.object) {
zend_get_gc_buffer_add_obj(gc_buffer, object->u.callback_filter.object);
}
if (object->u.callback_filter.closure) {
zend_get_gc_buffer_add_obj(gc_buffer, object->u.callback_filter.closure);
}
Copy link
Member

Choose a reason for hiding this comment

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

There is possibly a use-case for a zend_fcc_gc function

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 tried to add it to zend_gc.h but got some unresolved type issue :/ and I don't understand how it has no issue finding HashTable and zend_object

Copy link
Member

Choose a reason for hiding this comment

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

It makes sense to define the function outside of zend_gc.h 👍 This is what we do for the get_gc object handler for instance.

@Girgias
Copy link
Member Author

Girgias commented Oct 31, 2022

This seems sensible, but I would try to use the new APIs in a few additional use-cases, to validate them

I used it in ext-SQLite3 to further validate it.

Comment on lines 1042 to 1043
zend_fcc_addref(&fcc);
memcpy(&collation->cmp_func, &fcc, sizeof(zend_fcall_info_cache));
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it make sense to introduce a macro/inline function for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

That make sense, how about zend_fcc_copy()? Or should dup be used instead?

Copy link
Member

Choose a reason for hiding this comment

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

dup sounds like a good choice!

@Girgias Girgias force-pushed the fcc-closure branch 2 times, most recently from 153f531 to c405f03 Compare November 1, 2022 03:13
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