Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:119326
MIME-Version: 1.0
References: <Y8U9B4AVP/R7vazI@swift.blarg.de> <CAGBsUrdQAXE2qk_UDQY9v6pYpWH+=bX6S3ou+sttQamtDC1F9A@mail.gmail.com>
 <Y8VI5yanzli8D4Cm@swift.blarg.de> <alpine.DEB.2.23.453.2301181536160.23089@singlemalt.home.derickrethans.nl>
 <Y8gZWLQd1SvL6a3n@swift.blarg.de> <8DA390C5-5E67-4847-A89F-1A8CCC6C5389@gmail.com>
In-Reply-To: <8DA390C5-5E67-4847-A89F-1A8CCC6C5389@gmail.com>
Date: Wed, 18 Jan 2023 17:51:53 +0000
Message-ID: <CAGBsUrcHNeMYC1bSUkRgp2YNWjQJFojBxw43gcy3dkzEZfF4wg@mail.gmail.com>
To: internals@lists.php.net
Content-Type: multipart/alternative; boundary="0000000000006a384d05f28d799d"
Subject: Re: [PHP-DEV] RFC: rules for #include directives
From: tekiela246@gmail.com (Kamil Tekiela)

--0000000000006a384d05f28d799d
Content-Type: text/plain; charset="UTF-8"

I am in both camps. I think some slight cleanup might be in order, but not
how you are proposing it. First of all:
- I am against forward struct declarations. I think they decrease code
readability and should be avoided.
- I am against putting comments on #includes. Comments are noise in code
and often go out of date. Especially in a situation like this where the
comment is physically far away from the code that introduced it. To take
the example:
> #include "zend_portability.h" // for BEGIN_EXTERN_C
What if in future the need for BEGIN_EXTERN_C disappears? Who is going to
remember to update the comment?

As you said yourself, this refactoring has no practical effect on
compilation times. But your RFC states this as one of the advantages. This
is misleading.
Another advantage listed is "more correct code". Perhaps it is, but does it
mean that the current situation is likely to cause more frequent PHP bugs?
Would this refactoring help in finding PHP bugs more quicker?

Others have raised a valid point that PHP bugs are fixed in earlier
versions and then merged up into master. What you are proposing is going to
lead to issues with merging. And I don't mean merge conflicts, because
includes rarely change in bug fixes. I mean things like a bug fix using a
symbol that hasn't been used in the file but is included through one of the
more generic headers. Once such a commit is merged into master, it may turn
out that the symbol lacks a new include. This adds unnecessary work for bug
fixers who will now have to verify this and find the appropriate include
during merges.

So maybe reduce scope of this refactoring? Do it only where it's strictly
necessary. Don't use forward declarations or comments. Or maybe have all
ZEND headers included with a single header? Refactoring needs to make
developers' life easier and not just be done for the sake of following
best-practice.

--0000000000006a384d05f28d799d--