#include cleanup #10216

MaxKellermann · 2023-01-04T09:56:23Z

This PR is the start of an effort to reduce header bloat in the PHP code base. My idea is: in each .c file, include its own .h file first, and ensure that the header includes only a minimal set of other headers, possibly using forward declarations.
This increases code correctness and reduces compile times.
If you agree with this idea, I'll post more PRs.

Add missing #include lines to the header, so it can always be included without figuring out its header dependencies.

In the C file, include the header first so missing #includes are detected by the compiler, and use lighter header dependencies in the header, to speed up compile times.

MaxKellermann · 2023-01-04T11:51:20Z

Travis failure unrelated to this PR.

Girgias

Thank you for tackling this!

I think this makes a lot of sense, and I've been annoyed at this in the past when trying to figure out where what is defined.

But maybe wait for the opinion of someone else?

devnexen

LCTM

MaxKellermann · 2023-01-04T13:26:19Z

I've been annoyed at this in the past when trying to figure out where what is defined

Me too. Many of the "core" headers are really messed up (organic growth over two decades). Maybe I'll try to move some of the stuff to separate mini-headers to avoid slurping in everything everywhere.

cmb69 · 2023-01-04T13:50:52Z

My idea is: in each .c file, include its own .h file first, and ensure that the header includes only a minimal set of other headers, possibly using forward declarations.

So basically Plan 9 style includes "light". I think we can do that, but need to be careful not to break too many external extensions/SAPIs etc. At least we need to prominently document any major change in UPGRADING.INTERNALS.

cmb69 · 2023-01-04T13:51:45Z

cc @dstogov, @remicollet

MaxKellermann · 2023-01-04T13:57:20Z

need to be careful not to break too many external extensions/SAPIs etc.

Lots of the internal extensions break because e.g. they forget to include <errno.h> but errno is used everywhere. That's obviously a bug in the extension, but it was invisible previously, because zend_operators.h "helpfully" includes errno.h without actually using anything of it.

cmb69 · 2023-01-04T15:28:59Z

Lots of the internal extensions break because e.g. they forget to include <errno.h> but errno is used everywhere. That's obviously a bug in the extension, but it was invisible previously, because zend_operators.h "helpfully" includes errno.h without actually using anything of it.

Then we should at least add a note to UPGRADING.INTERNALS that the header includes have been changed, and maybe hint at this particular issue.

MaxKellermann · 2023-01-04T15:47:38Z

Then we should at least add a note to UPGRADING.INTERNALS that the header includes have been changed, and maybe hint at this particular issue.

Added to #10220

dstogov · 2023-01-09T06:04:01Z

I don't think we should include the comments like // for BEGIN_EXTERN_C (and similar). The are good for review only.

I'm indifferent to these changes and don't object.

MaxKellermann · 2023-01-09T08:09:13Z

I don't think we should include the comments like // for BEGIN_EXTERN_C (and similar). The are good for review only.

IMO every #include needs a justification, and documenting that justification is a good thing (forever, not just for review).

derickr · 2023-01-12T12:17:06Z

I don't think we should include the comments like // for BEGIN_EXTERN_C (and similar). The are good for review only.

IMO every #include needs a justification, and documenting that justification is a good thing (forever, not just for review).

FWIW, I disagree with this. Mostly, because these will never get updated so that this information becomes stale really quickly.

MaxKellermann · 2023-01-12T12:22:44Z

Mostly, because these will never get updated so that this information becomes stale really quickly.

Do you believe that nobody should ever write any code comments, API documentation and user manuals, because this information becomes stale just as quickly as code comments on #include lines?

Everybody can have their opinion, but yours sound rather unconvincing to me.

MaxKellermann · 2023-01-12T12:26:04Z

PS: if the explanation of an #include line becomes stale (which, of course, will happen all the time), it should be updated or the line should be removed.
Tools like iwyu can help with that.
But stale information doesn't cause any harm, and maybe-stale information is still better than no information at all.

morrisonlevi · 2023-01-15T16:28:34Z

Just chiming in: I agree with header cleanup, but not documenting why it's used. As Derick says, these often are not maintained, partially because if some header already included and I use something else now, why would I even think to go update that list?

Thanks for cleaning it up!

Girgias · 2023-01-15T16:56:06Z

I'm not sure if I agree, having at least an initial reason as to why something is included is IMHO better than guessing why it exists. Moreover, how often are these files actually modified and the stated reason of the include becomes invalid? Some of these files are basically frozen because of their simplicity and "finished" implementation.

dstogov · 2023-01-16T07:37:39Z

There are may be hundreds of reasons to include a single file. It's not possible to maintain all of them. Documenting only a single reason can often be incorrect. Any changes in includes may violate these kind of include comments.

I don't know where is our PHP @internal code style doc now (I can't find with google), but it definitely didn't recommend any include comments. If we are going to change our coding style, lets go through RFC and voting process.

@iluuu1994 @derickr @cmb69 @ramsey @saundefined @adoy ^^

Cf. <#10220 (comment)>. This reverts commit e628c66.

cmb69 · 2023-01-16T11:32:58Z

I don't know where is our PHP @internal code style doc now (I can't find with google)

It is in this repo: https://github.com/php/php-src/blob/master/CODING_STANDARDS.md

Anyway, I have reverted the four PRs we had for these changes. Let's see what comes out of #10338 and the RFC. :)

smalyshev · 2023-02-01T09:56:35Z

Zend/zend_alloc.h

-#include "../TSRM/TSRM.h"
-#include "zend.h"
+#include "zend_portability.h" // for BEGIN_EXTERN_C
+#include "zend_types.h" // for zend_result


The issue I have with such comments is that they are very fragile. As the code changes, it may use more of the things in this file. It may stop using zend_result. zend_result may be moved to another header file or cease to exist. But unless the file is going to go away completely, or similar major change happens, nobody would bother to update all comments of this nature. It is not realistic to expect from people who alter the code to evaluate each time whether their code was the last instance of using some particular construct introduced in some include, and whether now we need to change the comment to some other construct also introduced by that include - nobody ever is going to do that. So this may be technically true now - but in a year, we won't know if it's true, and in three years, it becomes just meaningless.

That would be RFC vote "Is it allowed to document an #include line with a code comment?" - https://wiki.php.net/rfc/include_cleanup - please vote!
See also #10472

Zend/zend_portability.h

MaxKellermann changed the title ~~Include cleanup~~ #include cleanup Jan 4, 2023

github-actions bot added the Category: Engine label Jan 4, 2023

MaxKellermann added 5 commits January 4, 2023 11:01

Zend/zend_portability: include php_config.h for HAVE_*

ac7dcce

Zend/zend_sort: include cleanup

47bfb40

Add missing #include lines to the header, so it can always be included without figuring out its header dependencies.

Zend/zend_alloc: include cleanup

9b7b320

In the C file, include the header first so missing #includes are detected by the compiler, and use lighter header dependencies in the header, to speed up compile times.

Zend/zend_string: include cleanup

f572a8a

Zend/zend_hash: include cleanup

e39ba0b

MaxKellermann force-pushed the include_cleanup branch from 042540b to e39ba0b Compare January 4, 2023 10:02

Girgias approved these changes Jan 4, 2023

View reviewed changes

devnexen approved these changes Jan 4, 2023

View reviewed changes

devnexen merged commit e628c66 into php:master Jan 4, 2023

MaxKellermann deleted the include_cleanup branch January 4, 2023 13:27

MaxKellermann mentioned this pull request Jan 4, 2023

#include cleanup 2 #10220

Merged

MaxKellermann mentioned this pull request Jan 10, 2023

#include cleanup part 3 #10279

Merged

shivammathur mentioned this pull request Jan 12, 2023

setup-php claims that Xdebug is available for PHP 8.3.0-dev when it is not shivammathur/setup-php#685

Closed

5 tasks

cmb69 added a commit that referenced this pull request Jan 16, 2023

Revert "#include cleanup (#10216)"

2d3427c

Cf. <#10220 (comment)>. This reverts commit e628c66.

MaxKellermann mentioned this pull request Jan 16, 2023

include cleanup part 5 #10345

Closed

smalyshev reviewed Feb 1, 2023

View reviewed changes

Zend/zend_portability.h Show resolved Hide resolved

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#include cleanup #10216

#include cleanup #10216

MaxKellermann commented Jan 4, 2023 •

edited

Loading

MaxKellermann commented Jan 4, 2023

Girgias left a comment

devnexen left a comment

MaxKellermann commented Jan 4, 2023

cmb69 commented Jan 4, 2023

cmb69 commented Jan 4, 2023

MaxKellermann commented Jan 4, 2023

cmb69 commented Jan 4, 2023

MaxKellermann commented Jan 4, 2023

dstogov commented Jan 9, 2023

MaxKellermann commented Jan 9, 2023

derickr commented Jan 12, 2023

MaxKellermann commented Jan 12, 2023

MaxKellermann commented Jan 12, 2023

morrisonlevi commented Jan 15, 2023 •

edited

Loading

Girgias commented Jan 15, 2023

dstogov commented Jan 16, 2023

cmb69 commented Jan 16, 2023

smalyshev Feb 1, 2023 •

edited

Loading

MaxKellermann Feb 1, 2023

#include cleanup #10216

#include cleanup #10216

Conversation

MaxKellermann commented Jan 4, 2023 • edited Loading

MaxKellermann commented Jan 4, 2023

Girgias left a comment

Choose a reason for hiding this comment

devnexen left a comment

Choose a reason for hiding this comment

MaxKellermann commented Jan 4, 2023

cmb69 commented Jan 4, 2023

cmb69 commented Jan 4, 2023

MaxKellermann commented Jan 4, 2023

cmb69 commented Jan 4, 2023

MaxKellermann commented Jan 4, 2023

dstogov commented Jan 9, 2023

MaxKellermann commented Jan 9, 2023

derickr commented Jan 12, 2023

MaxKellermann commented Jan 12, 2023

MaxKellermann commented Jan 12, 2023

morrisonlevi commented Jan 15, 2023 • edited Loading

Girgias commented Jan 15, 2023

dstogov commented Jan 16, 2023

cmb69 commented Jan 16, 2023

smalyshev Feb 1, 2023 • edited Loading

Choose a reason for hiding this comment

MaxKellermann Feb 1, 2023

Choose a reason for hiding this comment

MaxKellermann commented Jan 4, 2023 •

edited

Loading

morrisonlevi commented Jan 15, 2023 •

edited

Loading

smalyshev Feb 1, 2023 •

edited

Loading