Skip to content

Minor cleanups in Zend execution APIs #10699

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

Merged
merged 3 commits into from
Feb 26, 2023
Merged

Minor cleanups in Zend execution APIs #10699

merged 3 commits into from
Feb 26, 2023

Conversation

nielsdos
Copy link
Member

@nielsdos nielsdos commented Feb 25, 2023

Two minor cleanups, descriptions:

  • This check is always false because of the undefined behaviour rule that
    says a NULL pointer must never be dereferenced: we already dereference name
    when checking the cache slot, before the NULL check. So the NULL may be
    optimised away by the compiler. It looks like the code isn't even
    supposed to work with name being NULL, so just remove the check.

  • Remove always-true check in zend_fetch_static_property_address_ex()

  • Simplify always-true conditions.

This check is always false because of the undefined behaviour rule that
says a NULL pointer must never be dereferenced: we already dereference name
when checking the cache slot, before the NULL check. So the NULL may be
optimised away by the compiler. It looks like the code isn't even
supposed to work with name being NULL, so just remove the check.
@nielsdos
Copy link
Member Author

Failure in LINUX_X64_RELEASE_ZTS is oci_error() when oci_connect() fails [ext/oci8/tests/error1.phpt], so it's unrelated.

@devnexen devnexen requested a review from kocsismate February 26, 2023 08:30
if (name == NULL || !ZSTR_LEN(name)) {
if (!ZSTR_LEN(name)) {
Copy link
Member

Choose a reason for hiding this comment

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

Might make sense to add a ZEND_ASSERT(name); at the top of the function.

@Girgias Girgias merged commit 9108a32 into php:master Feb 26, 2023
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