Skip to content

Generated arginfo header files: remove empty zend_function_entry arrays #15705

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 2 commits into from
Sep 3, 2024

Conversation

DanielEScherzer
Copy link
Member

When a class (or enum) has no methods, rather than using an array that only contains ZEND_FE_END, use NULL for the functions. The implementation of class registration for internal classes, do_register_internal_class() in zend_API.c, already skips classes where the functions are NULL. By removing these unneeded arrays, we can reduce the size of the header files, while also removing an unneeded call to zend_register_functions() for each internal class with no extra methods.

When a class (or enum) has no methods, rather than using an array that only
contains `ZEND_FE_END`, use `NULL` for the functions. The implementation of
class registration for internal classes, `do_register_internal_class()` in
zend_API.c, already skips classes where the functions are `NULL`. By removing
these unneeded arrays, we can reduce the size of the header files, while also
removing an unneeded call to zend_register_functions() for each internal class
with no extra methods.
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.

ack for ext/random

Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

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

Ack for my stuff.
In general seems right overall, but needs more ACKs of course.
I actually thought about doing this myself some time ago, but never got to it. Thanks for this.

Copy link
Member

@devnexen devnexen left a comment

Choose a reason for hiding this comment

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

good to go for sockets, pgsql, pcntl, intl, gd

@SakiTakamachi
Copy link
Member

SakiTakamachi commented Sep 2, 2024

ACK for pdo, pdo_dblib, pdo_odbc, odbc, mysqli and sqlite3.
Thank you!

BTW, as one of 8.4 RM, I believe this is a okay to include in 8.4 as it is not a userland impacting change.

@DanielEScherzer
Copy link
Member Author

ACK for pdo, pdo_dblib, pdo_odbc, odbc, mysqli and sqlite3. Thank you!

BTW, as one of 8.4 RM, I believe this is a okay to include in 8.4 as it is not a userland impacting change.

Glad to hear it can be included - I have a few other ideas for improvements to the generated arginfo files but to avoid merge conflicts I'm going to wait until after this is (hopefully) merged before sending the next one.

@kocsismate kocsismate merged commit 53cb896 into php:master Sep 3, 2024
10 checks passed
@DanielEScherzer DanielEScherzer deleted the empty-methods branch September 3, 2024 22:19
@TimWolla
Copy link
Member

TimWolla commented Apr 9, 2025

I wonder if a:

#define {$functionEntryName} NULL

or

static const *{$functionEntryName} = NULL;

would have been preferable. I've currently got a case of an extension that currently doesn't have any global functions, thus requiring me to explicitly pass NULL in the zend_module_entry. Should any global function be added in the future, I would need to remember to not just change the stub file, but also the zend_module_entry to make use of ext_functions there.

@kocsismate
Copy link
Member

@TimWolla Yes, I think your idea makes a lot of sense!

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.

6 participants