On Mon, 16 Jan 2023 at 12:03, Max Kellermann <max+...@blarg.de> wrote:
> Hi, > > in the past weeks, I submitted four PRs for cleaning up the #includes > in the PHP code base: > > https://github.com/php/php-src/pull/10216 > https://github.com/php/php-src/pull/10220 > https://github.com/php/php-src/pull/10279 > https://github.com/php/php-src/pull/10300 > > I saw that the existing #include directives were inconsistent, > incomplete and bloated; things just worked by chance, not by design, > because there were a few headers which just included everything. I > wanted to help clean up this mess that had accumulated over two > decades. > > All PRs were welcomed by different reviewers and were merged; there > was just one minor criticism by Dmitry Stogov who thought the code > comments explaining many non-obvious #includes should be removed: > > https://github.com/php/php-src/pull/10216#issuecomment-1375140255 > > > 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. > > Yesterday, when a DTrace-specific regression was reported > (https://github.com/php/php-src/pull/10220#issuecomment-1383035139), > after which Dmitry asked to revert the whole PR: > > https://github.com/php/php-src/pull/10220#issuecomment-1383658247 > > Instead, I submitted a trivial fix for the regression > (https://github.com/php/php-src/pull/10334), which was rejected by > Dmitry > (https://github.com/php/php-src/pull/10220#issuecomment-1383706602) > but confirmed by the original reporter > (https://github.com/php/php-src/pull/10220#issuecomment-1383802334). > > In spite of that, Dmitry demanded to revert all of my #include > cleanups > (https://github.com/php/php-src/pull/10220#issuecomment-1383739816): > > > I'm asking to revert all these include cleanup commits! This is > > just a useless permutation. e.g. 68ada76 adds typedef struct _* that > > we didn't need before. How is this clearly? > > ... which so far only Derick Rethans agreed to: > > https://github.com/php/php-src/pull/10220#issuecomment-1383784480 > > > FWIW, I agree with Dmitry here, and these should all be reverted. It > > adds nothing but clutter. > > Christoph M. Becker performed the revert and suggested doing an RFC > (https://github.com/php/php-src/pull/10220#issuecomment-1383789100) > and vote on it. > > So this is a first draft of my proposal which I'd like to discuss with > you: > > https://github.com/php/php-src/pull/10338 > > Max > As the person who has signed off on the PRs and merged them, I am in favour of these changes and think the revert is a total over reaction. Before merging the first set, it was asked ahead of time if those changes were OK, and multiple people were in favour of these changes. The fact the DTrace build was broken inadvertently and only found out today is a failure of our CI Build matrix and myself for not knowing this option nor compiling with it. Not of the changes. Moreover, having those sorts of changes be RFCs seems counterproductive as the only people who care about this are actual core and extensions developers and this opens the gate for petty RFCs to resolve coding style disagreements. Finally, some of the wording used in those PR replies are far from civil and can push away new contributors to the language. Best regards, George P. Banyard
