ext/opcache: minor refactoring (2026-03)#21397
Conversation
arnaud-lb
left a comment
arnaud-lb
left a comment
There was a problem hiding this comment.
Looks good to me apart from Tim's comment
iluuu1994
left a comment
iluuu1994
left a comment
There was a problem hiding this comment.
Looks ok to me, apart from the comment made by Tim.
ext/opcache/ZendAccelerator.c
Outdated
| for (i = 0; i < entry->dependencies_count; i++) { | ||
| zend_class_entry *ce = zend_lookup_class_ex(entry->dependencies[i].name, NULL, 0); | ||
| for (uint32_t i = 0; i < entry->dependencies_count; i++) { | ||
| const zend_class_entry *dependent_ce = zend_lookup_class_ex(entry->dependencies[i].name, NULL, 0); |
There was a problem hiding this comment.
Nit: Technically the name dependent_ce indicates a dependency of dependent_ce on ce, rather than the other way around.
There was a problem hiding this comment.
Do you have an idea for a better name? dependency_of_ce maybe?
dstogov
left a comment
dstogov
left a comment
There was a problem hiding this comment.
I don't care about this.
If @iluuu1994 approves this, I don't object.
Girgias
force-pushed
the
opcache-minor-refactoring-2026-03
branch
from
ae63da1 to
964b903
Compare
ext/opcache/ZendAccelerator.c
Outdated
There was a problem hiding this comment.
Should be?
| zend_result ret; |
There was a problem hiding this comment.
And scope can also massively be reduced.
There was a problem hiding this comment.
It's not really reducing scope, just merging declaration and assignment tho.
| if (found && entry->dependencies) { | ||
| for (i = 0; i < entry->dependencies_count; i++) { | ||
| zend_class_entry *ce = zend_lookup_class_ex(entry->dependencies[i].name, NULL, ZEND_FETCH_CLASS_NO_AUTOLOAD); | ||
| const zend_class_entry *dependent_ce = zend_lookup_class_ex(entry->dependencies[i].name, NULL, ZEND_FETCH_CLASS_NO_AUTOLOAD); |
There was a problem hiding this comment.
I see the rename locally :| Might have pushed incorrectly.
There was a problem hiding this comment.
There are two cases. This one is still wrong.
TimWolla
left a comment
TimWolla
left a comment
There was a problem hiding this comment.
Not really my area of expertise, but didn't notice any obvious errors that were not already pointed out. Soft-LGTM.
Girgias
force-pushed
the
opcache-minor-refactoring-2026-03
branch
from
964b903 to
e75a912
Compare
Girgias
force-pushed
the
opcache-minor-refactoring-2026-03
branch
from
e75a912 to
e8cb781
Compare
5 participants
Commits should be reviewed in order.