Skip to content

#include cleanup part 4 #10300

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 20 commits into from
Jan 15, 2023
Merged

#include cleanup part 4 #10300

merged 20 commits into from
Jan 15, 2023

Conversation

MaxKellermann
Copy link
Contributor

No description provided.

devnexen
devnexen approved these changes Jan 13, 2023
View reviewed changes
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.

Looks about right to me but willing to give someone else the time to look into it.

Girgias
Girgias reviewed Jan 13, 2023
View reviewed changes
Zend/zend_weakrefs.c
Comment on lines 17 to +21
#include "zend_weakrefs.h"
#include "zend_API.h" // for ZEND_BEGIN_ARG_INFO_EX
#include "zend_objects.h" // for zend_object_std_init()
#include "zend_weakrefs_arginfo.h"
#include "zend_interfaces.h" // for zend_create_internal_iterator_zval()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#include "zend_weakrefs.h"
#include "zend_API.h" // for ZEND_BEGIN_ARG_INFO_EX
#include "zend_objects.h" // for zend_object_std_init()
#include "zend_weakrefs_arginfo.h"
#include "zend_interfaces.h" // for zend_create_internal_iterator_zval()
#include "zend_weakrefs.h"
#include "zend_weakrefs_arginfo.h"
#include "zend_API.h" // for ZEND_BEGIN_ARG_INFO_EX
#include "zend_objects.h" // for zend_object_std_init()
#include "zend_interfaces.h" // for zend_create_internal_iterator_zval()

I think having the arginfo next to the corresponding file header is "better" except if there is a reason not to? As including an arginfo header makes it clear very early on that we are dealing with either a class or an extension defining functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved zend_weakrefs_arginfo.h to the top (right below its own public header) because this is the only source that includes zend_weakrefs_arginfo.h, and thus nobody can really verify whether that header is self-containing, therefore moving it up as much as possible will have the fewest other includes already cluttering the namespace. But that's rather unimportant.

Most of the time, I tried to sort includes in alphabetic order if there were no other reasons to do something else.

@MaxKellermann
Zend/zend_execute: always include zend_globals.h
06c502b 
Commit ecc880f was incomplete; EG() is used in inline functions
outside of ZEND_DEBUG.
@MaxKellermann
ext/opcache/ZendAccelerator.h: add missing include for "INIT_FUNC_ARGS"
9de7840
@MaxKellermann
Zend/zend_globals_macros: add missing include for BEGIN_EXTERN_C
89c86f1
@MaxKellermann
main/php.h: add missing includes
0226f70
@MaxKellermann
Zend/zend_language_scanner: include cleanup
5ec6158
@MaxKellermann
Zend/zend_ast: include cleanup
0b55055
@MaxKellermann
Zend/zend_call_stack: include cleanup
139568f
@MaxKellermann
Zend/zend_iterators: include cleanup
101e86d
@MaxKellermann
Zend/zend_compile: include cleanup
c7b481f
@MaxKellermann
Zend/zend_weakrefs: include cleanup
ab21953
@MaxKellermann
Zend/zend_enum: include cleanup
1324306
@MaxKellermann
Zend/zend_interfaces: include cleanup
1c4a2d4
@MaxKellermann
Zend/zend_observer: include cleanup
3a0b204
@MaxKellermann
Zend/zend_llist: include cleanup
f6668f3
@MaxKellermann
Zend/zend_gc: include cleanup
bb41217
@MaxKellermann
Zend/zend_objects: include cleanup
b3a619d
@MaxKellermann
Zend/zend_list: include cleanup
189fcb7
@MaxKellermann
Zend/zend_API: include cleanup
e73c126
@MaxKellermann
Zend/zend_inheritance: include cleanup
cdeb918
@MaxKellermann
Zend/zend_closures: include cleanup
038b317
@MaxKellermann
Copy link
Contributor Author

Thanks again @Girgias for your pedantic eyes finding every stupid mistake I leave in there... :-)

@Girgias Girgias merged commit 68ada76 into php:master Jan 15, 2023
@MaxKellermann MaxKellermann deleted the include_cleanup4 branch January 16, 2023 08:32
cmb69 added a commit that referenced this pull request Jan 16, 2023
@cmb69
Revert GH-10300
bf1cfc0 
Cf. <#10220 (comment)>.

This reverts commit 68ada76.
his reverts commit 45384c6.
This reverts commit ef7fbfd.
This reverts commit 9b9ea0d.
This reverts commit f15747c.
This reverts commit e883ba9.
This reverts commit 7e87551.
This reverts commit 921274d.
This reverts commit fc1f528.
This reverts commit 0961715.
This reverts commit a93f264.
This reverts commit 72dd94e.
This reverts commit 29b2dc8.
This reverts commit 05c7653.
This reverts commit 5190e5c.
This reverts commit 6b55bf2.
This reverts commit 184b4a1.
This reverts commit 4c31b78.
This reverts commit d44e968.
This reverts commit 4069a5c.
@MaxKellermann MaxKellermann mentioned this pull request Jan 16, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Girgias Girgias Girgias left review comments

@devnexen devnexen devnexen approved these changes

Assignees
No one assigned
Labels
Category: Engine Extension: opcache
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MaxKellermann @devnexen @Girgias