-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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
Improve FCC API #9840
Conversation
There was a problem hiding this 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
ext/spl/spl_iterators.c
Outdated
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); | ||
} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
I used it in ext-SQLite3 to further validate it. |
ext/sqlite3/sqlite3.c
Outdated
zend_fcc_addref(&fcc); | ||
memcpy(&collation->cmp_func, &fcc, sizeof(zend_fcall_info_cache)); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
153f531
to
c405f03
Compare
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.