Skip to content
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

reflection: try to avoid manual string comparison #17726

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

Conversation

DanielEScherzer
Copy link
Contributor

In is_closure_invoke() compare with the well known zend_string ZEND_STR_MAGIC_INVOKE rather than the underlying literal value.

In `is_closure_invoke()` compare with the well known zend_string
`ZEND_STR_MAGIC_INVOKE` rather than the underlying literal value.
Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

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

There is another one in

if (ce == zend_ce_closure && orig_obj && (method_name_len == sizeof(ZEND_INVOKE_FUNC_NAME)-1)
&& memcmp(lcname, ZEND_INVOKE_FUNC_NAME, sizeof(ZEND_INVOKE_FUNC_NAME)-1) == 0
and also some in Zend/zend_builtin_functions.c, can you fix those as well?

Make use of the well known zend_string objects `ZEND_STR_MAGIC_INVOKE` and
`ZEND_STR_UNKNOWN_CAPITALIZED`
@DanielEScherzer
Copy link
Contributor Author

There is another one in

if (ce == zend_ce_closure && orig_obj && (method_name_len == sizeof(ZEND_INVOKE_FUNC_NAME)-1)
&& memcmp(lcname, ZEND_INVOKE_FUNC_NAME, sizeof(ZEND_INVOKE_FUNC_NAME)-1) == 0

and also some in Zend/zend_builtin_functions.c, can you fix those as well?

So I tried to fix the other case you found in reflection, but that one is really complicated, since the source being compared is also a char array, and its not clear how to get a zend_string in the case when the char array was extracted from the broader string of SomeClass::someMethod.

I addressed the builtin functions, but I'll note that using zend_string_equals_ci probably doesn't help much (and, it might even be worse since two extra ZSTR_LEN calls and an extra ZSTR_VAL call are used, while zend_string_equals_literal_ci uses the literal values and sizeof which can probably be evaluated by the compiler rather than waiting until runtime)

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.

2 participants