ext/spl: refactor class autoloading to use newer FCC APIs #20841
Conversation
ext/spl/php_spl.c
Outdated
| } | ||
|
|
||
| zend_hash_next_index_insert_ptr(spl_autoload_functions, alfi); | ||
| zend_fcall_info_cache *entry = emalloc(sizeof(zend_fcall_info_cache)); |
There was a problem hiding this comment.
Could use zend_hash_next_index_insert_mem here
There was a problem hiding this comment.
I don't think this is correct, as I need to do a "deep" copy of the FCC by increasing refcounts rather than a shallow dup. Will add a comment to explain this.
There was a problem hiding this comment.
You can if you use zend_fcc_addref instead of dup
There was a problem hiding this comment.
Indeed, not sure why I didn't think of that.
Remove unused parameter Use zend_string_concat2() API Use size_t for ext_len parameter type Return bool instead of int
Girgias
force-pushed
the
autoloader-fcc-api
branch
from
9304196 to
cb596b2
Compare
This is used only once anyway
Girgias
force-pushed
the
autoloader-fcc-api
branch
from
cb596b2 to
8ca47c8
Compare
|
@ndossche done your suggestion and saw something else that I could clean up so pushed one extra commit. :) |
|
|
||
| if (fcc.function_handler && zend_string_equals_literal( | ||
| fcc.function_handler->common.function_name, "spl_autoload_call")) { | ||
| if (zend_string_equals_literal(fcc.function_handler->common.function_name, "spl_autoload_call")) { |
There was a problem hiding this comment.
Don't you need to release the trampoline in this case?
There was a problem hiding this comment.
It shouldn't be possible for this function name to be a trampoline. I guess adding an IS_INTERNAL check would guarantee that this is true.
2 participants
Commits should be reviewed individually.
The primary motivation for me to introduce the newer FCC APIs was to make it easier to handle userland callables as the class autoloading system had a few bugs due to the complex semantics.
This is effectively taking the class autoloading part of #8294 as it is a straight-up code clarity improvement.