Skip to content

include cleanup part 6 #10410

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

Closed
wants to merge 22 commits into from
Closed

Conversation

MaxKellermann
Copy link
Contributor

@MaxKellermann MaxKellermann commented Jan 22, 2023

As suggested on php-internals, this PR contains parts of #10345 that are not so controversial, because they mostly add missing things, but do not remove anything just yet.

One new commit adds compatibility #includes to main/php.h, because people on php-internals have suggested that source compatibility with third-party extensions should be retained, even if those extensions are buggy (e.g. forgetting to include errno.h even though they use errno). Much of that commit addresses issues that will arise once more includes are cleaned up; it makes those header dependencies explicit, instead of making them accidental and implicit.

@@ -434,4 +434,18 @@ END_EXTERN_C()

#include "php_reentrancy.h"

/* the following headers used to be included indirectly, and we have
* them here only for backwards compatibility with thirdparty
* extensions */
Copy link
Member

Choose a reason for hiding this comment

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

+1 for this compromise.

@MaxKellermann
Copy link
Contributor Author

Rebased & moved a few more non-intrusive commits from #10345
I believe this PR should be merged even if the vote https://wiki.php.net/rfc/include_cleanup is not yet over.
Check commit c8ec2ed to see a similar type of code cleanup that was merged just yesterday, which was identical to one commit from #10345 (my commit was reverted, a new one by somebody else then got merged, duh!).

@MaxKellermann
Copy link
Contributor Author

CI failures due to bug in commit a211956; see #10527 for my proposed fix

@MaxKellermann
Copy link
Contributor Author

All #include comments removed, according to the RFC vote.

@MaxKellermann MaxKellermann deleted the include_cleanup6 branch March 6, 2023 04:24
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