Here are my comments about the last PR https://github.com/php/php-src/pull/10345 Max asked to repost them in this thread:
In my opinion this should not be accepted. Despite the intention is good, this huge patch doesn't improve anything but adds new questions. - It introduces new include files (e.g. zend_char.h, what was wrong with zend_portability.h?) - It adds a lot of forward declarations (e.g. typedef struct _<type> <type>) - It adds almost useless comments that are not goigg to be kept as to date (e.g. #include "zend_portability.h" // for BEGIN_EXTERN_C, zend_portability.h defines all base macros for Zend engine) - zend.c adds a number of commented includes (is this some kind of TODO?) - <stdint.h>> is included in many other *.h files instead of single zend_portability.h or zend_types.h. I see all these changes as a source permutation that don't add significant value. On the other hand: - this may introduce PHP build failures on some untested configurations (like it was with DTRACE) - we have to support several PHP branches at once and merging fixes between very different branches is going to introduce problems - this changes may break compilation of third-party software (e.g. xdebug). Not a huge problem, but it's not great to do this in a minor version update. On Mon, Jan 16, 2023 at 8:22 PM Levi Morrison via internals < internals@lists.php.net> wrote: > As commented yesterday on one of the PRs, I strongly agree with the > cleanups. Our code has implicit dependencies and if you reorder the > includes in the same file, you can break builds. That shouldn't ever > happen. > > I also agree that dtrace breaking is a failure of CI and testing, not > necessarily the patch (I haven't reviewed that PR carefully, maybe > there was some carelessness but if it's not tested, it's hard to blame > anyone). > > Lastly, I do not care for the comments which explain why a header is > included. My experience is that these comments bitrot quite quickly. > > Overall, I hope we can resume this effort. > > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: https://www.php.net/unsub.php > >
