Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:119528
MIME-Version: 1.0
References: <Y9pXdfDdYqnzwnIt@swift.blarg.de> <CAAfnsFU71TeGJJea+u0HJ0sHSLkUYUk6=zqLj01s0oF07BaBcQ@mail.gmail.com>
 <Y+ij4B3zkYYXIuUq@swift.blarg.de>
In-Reply-To: <Y+ij4B3zkYYXIuUq@swift.blarg.de>
Date: Sun, 12 Feb 2023 15:49:09 +0100
Message-ID: <CAAfnsFUrh4Pq89QbH-6YfK47vRS38ucyFh=89of3L6w=-F2oCQ@mail.gmail.com>
To: Max Kellermann <max+php@blarg.de>
Cc: Peter Kokot <petk@php.net>, internals@lists.php.net
Content-Type: text/plain; charset="UTF-8"
Subject: Re: [PHP-DEV] [VOTE] include cleanup
From: petk@php.net (Peter Kokot)

On Sun, 12 Feb 2023 at 09:31, Max Kellermann <max+php@blarg.de> wrote:
>
> On 2023/02/11 17:14, Peter Kokot <petk@php.net> wrote:
> > I've voted in favor of the RFC because of the code-cleaning,
> > tech-debt-reducing improvements to code readability.
>
> Exactly my point, and I'm surprised by the resistance.
>
> Not only surprised, but also disappointed that many have voted against
> code cleanup, but where have those people been when this was being
> discussed?
>
> Matthew said there had been "chatter from a number of folks after the
> changes were merged about builds no longer compiling", but was not
> able to render that more precisely.
>
> None of that was discussed on GitHub nor here on php-internals.  I
> have to question whether these build breakages even exist.
>
> (Other than the DTrace build failure which happened because one line
> was missing, but that's a fact and not "chatter", and one bug reporter
> and not "a number of folks".  Let's put this dead horse to rest.)
>
>
> > BTW, merging from PHP 8.1 up is not problematic. Git diff only looks
> > at a few lines of code above and below. Not the top of the file.
>
> This was the only counter-argument ever discussed here, and I'm
> puzzled that the imagination of merge conflicts scares so many people.
> About a kind of change that is unlikely to cause one.
>
> Any code change can cause a merge conflict, but include cleanups are
> the least likely cause of all, because include directives are almost
> never touched in bugfix-only branches.
>
>
> Is that all, or is there another, yet unnamed reason why there's so
> much resistance?  The hearsay about build failures?
>
>
> There are 3 more days to vote, and it's a tie currently - means 9
> "YES" votes missing for supermajority or else the RFC gets rejected.
> That rejection would not only be a missed chance to modernize the PHP
> code base, but also a sign to potential PHP contributors that the PHP
> maintainers don't value clean code.  This is unsettling.
>
> Imagine how this will overshadow future attempts to remove historical
> cruft from a decades-old code base, because the counter-arguments
> apply the same to any kind of code cleanup.  As any decades-old code
> base, there's a lot of historical cruft in PHP which gets in the way
> all the time, much more than a hypothetical one-time merge conflict.
> Historical cruft keeps piling up if you don't keep cutting it down all
> the time.
>
> Cleaner code is easier to read and understand, which makes existing
> bugs easier to fix and makes new bugs less likely to be added.  That
> outweighs, in my opinion, all the possible disadvantages that the
> process of code cleanup could possibly have, by orders of magnitude.
>
> Max

Well, the PHP long-term maintainers are just a bit stubborn when it
comes to such changes and probably what concerns them is writing code
styles in stone. We shouldn't conclude that the PHP team as a whole
doesn't care about clean code based on this voting. People just need a
bit of time to grasp the changes and discuss these things over a
longer period. They just care more about the stability of the current
repo and the extensions out there than the cosmetics of the ASCII
characters in files. I agree though, that both sides need to be
improved.

One more question here. Is the iwyu (include-what-you-use) tool used
here to clean up these header files?

I'm assuming something like this could be one day executed:
iwyu --no_fwd_decls --no-comments

Based on this, this is an awesome tool and can improve the code.

I'd say to go hand in hand here and one day later discuss things like
this again to integrate these gradually. Automating is always tricky
on the other hand. Like adding the iwyu step in the build checks.