-
Notifications
You must be signed in to change notification settings - Fork 7.8k
Zend: Move FCI/FCC/callables related functions to a dedicated header and C file #12240
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
base: master
Are you sure you want to change the base?
Conversation
af3fb15
to
61cc444
Compare
Is the motivation strong enough to move all this code? Merge conflicts are guaranteed, and |
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 don't understand the motivation of this code motion and I don't like to lose git history.
I'm against this kind of changes.
I don't understand the argument about the git history being lost, it still exists and even with Using
The |
61cc444
to
9676e30
Compare
This groups all related functions into one C file and gives a good overview of the API via the header, instead of it being spread around multiple different files.
I am currently not sure if I should not move
zend_fcc_addref()
andzend_call_known_fcc()
to the.c
file to reduce some header inclusions, as I'm not entirely sure if it makes a lot of sense for them to be always inlined.Primary motivation
Having the FCI and FCC structures in their own header to be able to include them in
zend_gc.h
to be able to movezend_get_gc_buffer_add_fcc()
intozend_gc.h
as it is otherwise not very discoverable.Future scope
Making the
zend_is_callable_ex()
private as it is only used byzend_call_function()
, the nonex
variant is used twice in php-src, however the PCNTL usage is bogus (as it doesn't do anything) and the usage insapi_windows_set_ctrl_handler()
is about not being able to attach the function, and could possibly be changed to be generic. Meaning, we could possibly also drop the nonex
variant.Another thing would be to drop the rather confusing FCI API which I started in #9723
One minor thing that may also be useful, is to drop the dependency on
zend_API.h
in the.c
file to not have a circular dependency.