Skip to content

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.

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.
Every single callsite already checks if it set to call this function, so no need to duplicate the work
@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

ZEND_API zend_function *zend_get_call_trampoline_func(const zend_class_entry *ce, zend_string *method_name, bool is_static) /* {{{ */
ZEND_API ZEND_ATTRIBUTE_NONNULL zend_function *zend_get_call_trampoline_func(
const zend_function *magic_call, zend_string *method_name, bool is_static) /* {{{ */
Copy link
Member

Choose a reason for hiding this comment

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

The is_static parameter should also go away then.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is used to add a flag within the function body, and there is no convenient way to extract this information otherwise.

Copy link
Member

Choose a reason for hiding this comment

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

You should be able to take this from magic_call->common.fn_flags just like the attribute flags.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants