Skip to content

Improve FCC API #9840

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

Merged
merged 7 commits into from
Nov 2, 2022
Merged

Improve FCC API #9840

merged 7 commits into from
Nov 2, 2022

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