Skip to content

zend_object_handler.c: call zend_get_call_trampoline_func() directly #20297

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

zend_object_handler.c: call zend_get_call_trampoline_func() directly #20297

wants to merge 2 commits into from
+12 −27

Conversation

@Girgias
Copy link
Member

@Girgias Girgias commented Oct 25, 2025

Abstracting away the bool parameter is not that useful, and doesn't make the code more legible.
So just get rid of them and call the function with the boolean parameter directly.

@Girgias Girgias marked this pull request as ready for review October 25, 2025 23:34
@Girgias Girgias requested a review from dstogov as a code owner October 25, 2025 23:34
TimWolla
TimWolla requested changes Oct 26, 2025
View reviewed changes
dstogov
dstogov reviewed Oct 27, 2025
View reviewed changes
Copy link
Member

@dstogov dstogov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably saves few re-loads but this won't make any visible effect for the expensive operation (function call).
I'm not sure if the modified API becomes more clear. Especially if we keep is_static argument.

@iluuu1994
Copy link
Member

I'm in favor if we can get rid of the is_static param, as Tim suggested. You can probably also reduce the diff by naming the args accordingly. I like small diffs, but if you think the new param name is better I'm fine with it.

@Girgias
zend_object_handler.c: call zend_get_call_trampoline_func() directly
0e5c257 
Abstracting away the bool parameter is not that useful, and doesn't make the code more legible.
So just get rid of them and call the function with the boolean parameter directly.
@Girgias
Pass zend_function* directly to zend_get_call_trampoline_func()
3012e04 
Every single callsite already checks if it set to call this function, so no need to duplicate the work.
We can also remove the is_static bool parameter as it can be derived from the fn_flags.
@Girgias
Copy link
Member Author

Girgias commented Oct 29, 2025

Got rid of is_static and reduced the diff by keeping the fbc name.

@Girgias Girgias requested a review from TimWolla October 29, 2025 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dstogov dstogov dstogov left review comments

@arnaud-lb arnaud-lb Awaiting requested review from arnaud-lb

@iluuu1994 iluuu1994 Awaiting requested review from iluuu1994

@TimWolla TimWolla Awaiting requested review from TimWolla

Assignees

No one assigned

Labels

ABI break Category: Engine

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Girgias @iluuu1994 @TimWolla @dstogov