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
On 2023/01/16 13:38, Kamil Tekiela wrote:
> Did you create an RFC already? Or is this RFC-like discussion?
No. I followed https://wiki.php.net/rfc/howto and this is step 1.
Creating the actual RFC is step 3.
(I'm new to PHP and this process - so far, I've only contributed PHP
code through
On 2023/01/16 17:25, Kamil Tekiela wrote:
> Have you done any benchmarking in terms of build time? Is there any
> tangible difference or is it only theoretical?
I did, but in this early stage, there is no measurable speedup yet,
because there are still too many "fat" headers left that need to be
On 2023/01/16 17:14, Chase Peeler wrote:
> will that have any impact on the final product such as reduced binary sizes
> or better performance?
No, this kind of code cleanup must not affect the resulting binary at
all, therefore no change to runtime behavior/performance.
Though the build will
On 2023/01/16 17:44, Tim Düsterhus wrote:
> Sorry for the duplicate mail, but it just came to my mind checking the CI
> build logs from before and after the revert:
I wish that were true numbers, but they're not - that's just (extreme)
noise. Later CI builds are back to shorter build times. I
On 2023/01/18 18:51, Kamil Tekiela wrote:
> > #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?
(I just addressed that concern in reply to Rowan's email.)
> As you said yourself, this
On 2023/01/18 18:51, Kamil Tekiela wrote:
> - I am against forward struct declarations. I think they decrease code
> readability and should be avoided.
btw. if this opinion is shared by the majority of voters, I'll send a
PR to remove all existing forward declarations from PHP.
There are many,
On 2023/01/18 18:06, Rowan Tommins wrote:
> Then I guess you've never heard people quoting Robert "Uncle Bob" Martin, who
> is well known for asserting that code should be self-documenting, and
> therefore not need comments, saying things like:
>
> > The proper use of comments is to compensate
On 2023/01/16 13:48, "G. P. B." wrote:
> 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.
How shall we
On 2023/01/18 12:51, "G. P. B." wrote:
> So I think creating an official RFC is the only way.
Okay then, I'm now at step 2 of https://wiki.php.net/rfc/howto
I registered the account "maxk" on the Wiki - please grant me RFC
karma!
Max
--
PHP Internals - PHP Runtime Development Mailing List
To
On 2023/01/18 13:11, "Christoph M. Becker" wrote:
> > I registered the account "maxk" on the Wiki - please grant me RFC
> > karma!
>
> Done. Best of luck!
Thanks.
Here's my RFC: https://wiki.php.net/rfc/include_cleanup
I hope that's correct this way - then we're now at step 5 "Listen to
the
On 2023/01/18 16:41, Derick Rethans wrote:
> Including a "random" zend header also sounds like a great benefit. I
> shouldn't need to care as an extension author which header enables which
> internal function(s) for usage.
So, in your opinion, choosing an arbitrary of the 95 Zend/zend*.h
On 2023/01/20 11:51, Michael Voříšek - ČVUT FJFI wrote:
> I would highly appreciate if this could be done automatically, ie.
> generate "the correct" includes and assert them by
> https://github.com/php/php-src/blob/php-8.2.1/.github/actions/verify-generated-files/action.yml
> CI. If you can
On 2023/01/20 11:52, Jakub Zelenka wrote:
> Reduce the diff to absolute minimum to prevent potential conflict. I think
> it would be more acceptable if there was a plan that will get us there in
> multiple releases rather than do one big bang change.
My draft PR currently contains 104 very small
On 2023/01/16 17:43, Max Kellermann wrote:
> I did, but in this early stage, there is no measurable speedup yet,
> because there are still too many "fat" headers left that need to be
> included everywhere, which in turn include too much. The many small
> improvemen
On 2023/01/20 10:53, Jakub Zelenka wrote:
> I'm afraid it's too late for PHP.
It's never too late. Don't give up PHP :-)
PHP is an old code base. I want to help modernize it.
> As it was said, this is problematic for bug fixes when merging up
> and it's extra hurdle for maintainers - read
On 2023/01/20 12:50, Jakub Zelenka wrote:
> That's exactly it's too big PR that is changing too much which might result
> exactly to the mentioned concerns about breaking some untested builds and
> it's very hard to track for dev what actually changed.
This huge PR isn't meant to be merged in
On 2023/01/20 13:41, Jakub Zelenka wrote:
> I think you should clarify in the RFC how this is going to be split and
> what's the time frame for getting that all in.
The RFC is not the actual cleanup, it's for reaching an agreement on
whether it is a desirable goal to have minimal includes
On 2023/01/18 15:55, Max Kellermann wrote:
> Here's my RFC: https://wiki.php.net/rfc/include_cleanup
Two weeks will be up the day after tomorrow, and there hasn't been any
further discussion for more than a week, so I figure there is no
further demand for discussing my RFC.
I have repl
On 2023/01/20 16:00, "Christoph M. Becker" wrote:
> I'm afraid the worst that can actually happen is that this breaks a lot
> of PHP extensions, SAPIs, etc.
True, I meant out-of-tree code, too.
I will add some extra care to ensure compatibility even with "bad"
extensions, by re-adding removed
On 2023/01/20 14:49, Tim Düsterhus wrote:
> A reasonable first step might be just *adding* all the missing '#include's
> to '*.c' with one PR per ext/* directory
Sounds like a good plan, but after the discussion about maintaining
compatibility with bad out-of-tree code, I believe this should
On 2023/03/01 13:15, Max Kellermann wrote:
> IMO this is a bug in the process, and I'm trying to fix it. It should
> be allowed to merge small incremental improvements without a dedicated
> RFC.
>
> This is the first draft of my RFC:
> https://wiki.php.net/rfc/code_optimizat
On 2023/02/22 13:45, Max Kellermann wrote:
> 13 years ago, there was commit
> https://github.com/php/php-src/commit/477649cd3f09 which attempted to
> fix this, but was reverted on the same day by commit
> https://github.com/php/php-src/commit/16450418b188 with just commit
> mess
Hi,
while working on https://github.com/php/php-src/pull/10663 I saw CI
failures because after that PR, the sanitizer flags were missing in
the linker call; they were only present in CFLAGS and LDFLAGS but not
in CXXFLAGS.
That is because ".cirrus.yml" sets LDFLAGS, but that value never gets
On 2023/02/19 11:56, Niels Dossche wrote:
> It's also worth noting that there's a couple of places where there's
> just a check for if (function()) { failure code }. i.e. there is no
> check for == FAILURE, it's just "implied".
Ouch. That's not only fragile, but also very obscure.
(Yadda
Hi,
I've done a bit of refactoring work around code using "zend_result",
but I keep wondering why it even exists.
It was added in 1999 by commit 573b46022c46 in a huge 16k line commit
(as macros, converted to enum in 2012 by commit e3ef84c59bf).
In 1999, C99 was brand new, and the "bool" type
On 2023/02/19 08:56, Nikita Popov wrote:
> If you have a function like zend_stream_open_function(), SUCCESS and FAILURE
> are directly meaningful values.
Agree, but that doesn't explain why FAILURE needs to be negative.
> The current guideline for use of bool and zend_result in php-src is that
On 2023/02/19 09:45, Nikita Popov wrote:
> I expect that there are two main reasons for that:
> - There are probably some places that return a (non-negative) value or
> FAILURE.
> - There are probably some places that check for success/failure using >= 0
> and < 0. Another POSIX-ism.
>
> I
Hi,
today, I stumbled on this piece of code in zend_persist.c:
zend_ast_ref *old_ref = Z_AST_P(z);
Z_AST_P(z) = zend_shared_memdup_put(Z_AST_P(z), sizeof(zend_ast_ref));
This is the definition of `zend_ast_ref` from zend_types.h:
struct _zend_ast_ref {
zend_refcounted_h gc;
/*zend_ast
On 2023/03/06 00:43, Juris Evertovskis wrote:
> The current amount of secondary votes makes it feel daunting. I would
> suspect not all voters will think thoroughly about each of the questions.
> I suggest that most of these questions could be agreed upon in discussions
> before the vote.
On 2023/02/16 08:59, Derick Rethans wrote:
> Secondary votes are irrelevant if the primary one doesn't pass.
You may be formally correct (or maybe not, because
https://wiki.php.net/rfc/voting doesn't really say that).
In any case, a vote that reaches supermajority (i.e. it would have
been
On 2023/02/16 17:52, Tim Düsterhus wrote:
> Not necessarily. It might've been the case that a voter believes that
> include cleanups should not happen, but at the same time believes that *if*
> cleanups happen, then splitting a header is a natural part of such a
> cleanup.
Maybe, but that seems
On 2023/02/22 22:09, Peter Kokot wrote:
> Most likely, some of those three unset lines could be removed today,
> yes, but I can't be sure. If removal would fix some bug, then probably
> it is time to remove them and see where things break afterwards.
https://github.com/php/php-src/pull/10678
On 2023/02/22 22:09, Peter Kokot wrote:
> If removal would fix some bug, then probably it is time to remove
> them and see where things break afterwards.
Observe the output of PHP's "./configure --help":
"Some influential environment variables:
[...]
LDFLAGS linker flags, e.g. -L if
On 2023/02/22 22:09, Peter Kokot wrote:
> >From my quick check, the unset is initially done to move the currently
> set flags to extra flags variable and clean the variable for further
> additions so there are no duplicate ones or something like that.
>
On 2023/02/23 05:53, Stanislav Malyshev wrote:
> It could be these changes do not make sense. It also could be they are
> necessary for the build to work on one of dozens of systems, distros and
> tools combinations PHP is being built on. Unless you have tested on all of
> them, I'd advise
On 2023/03/01 06:37, Max Kellermann wrote:
> Indeed it appears Dmitry is right - code refactoring is generally NOT
> allowed (unless there is an explicit RFC vote, and I havn't seen one).
IMO this is a bug in the process, and I'm trying to fix it. It should
be allowed to merge small incre
On 2023/02/28 21:16, Dmitry Stogov wrote:
> Recently we voted for inluce cleanup RFC
> https://wiki.php.net/rfc/include_cleanup and it was declined.
> Despite that a series of code refactoring commits from Max were silently
> merged into the master.
> As this is a violation of the community rules
On 2023/02/28 23:34, Dmitry Stogov wrote:
> > https://github.com/php/php-src/commit/b98f18e7c3838cf587a1b6d0f033b89e9909c79d
> >
> > No vote was made on this, therefore this doesn't violate any community
> > rules, does it?
> >
>
> Please reread https://wiki.php.net/RFC/voting#voting
> RFC is
On 2023/02/28 23:08, Dmitry Stogov wrote:
> > Which community rule was violated by whom?
> >
>
> Merging the things that were rejected. You may name this differently but
> this is still code refactoring.
That sidesteps my question, and answers something else.
> In your opinion, what exactly
On 2023/02/28 22:31, Dmitry Stogov wrote:
> https://github.com/php/php-src/commit/0270a1e54c0285fa3c89ee2b0120073ef57ab5fa
This kind of change was favored by a supermajority.
You argue that this supermajority vote is irrelevant, and formally it
indeed is, but pondering about formalities is kind
On 2023/02/28 23:33, Max Kellermann wrote:
> > Include cleanups RFC was rejected.
> > No refactoring RFC was presented.
> > A lot of changes that affect all core contributors are committed into
> > master.
>
> Do you mean to imply that code changes that do not imple
On 2023/02/11 17:14, Peter Kokot 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
On 2023/02/09 13:37, Rowan Tommins wrote:
> Firstly, let's try to keep this discussion civil, and assume good faith on
> both sides. Parts of your e-mail read like accusations of bad behaviour,
> rather than genuinely trying to understand what happened, and how we can
> collectively avoid it
On 2023/02/09 14:49, Michael Voříšek - ČVUT FJFI wrote:
> One good way to maintain some quality standard is to enforce it thru CI
Agree, the CI is a nice tool for enforcing certain policies, but first
there needs to be a decision on what is the desired quality standard.
Finding such a decision
Hi,
what happens if there is a bug in a vendored library, but upstream
refuses to fix it?
Last month, my PR for removing obsolete C99 integer checks was merged:
https://github.com/php/php-src/pull/10304
This change speeds up configure and removes code that violates the C99
spec.
It included
On 2023/02/09 16:29, Rowan Tommins wrote:
> This is where I'm suggesting you assume good faith: what looks like a
> "secret revert" probably feels like something entirely different to Derick.
Timeline:
- Jan 13 11:34 AM: PR https://github.com/derickr/timelib/pull/141/files
- Jan 18 4:34 PM: PR
On 2023/02/09 19:04, Tim Düsterhus wrote:
> However based on the discussion of the RFC I believe that voters may have
> assumed that a "No" means "A cleanup is not allowed", because several
> participants expressed an active aversion to a cleanup during the
> discussion. As for myself I've
On 2023/02/09 23:09, Matthew Weier O'Phinney wrote:
> I'm not directly involved in maintenance, but my take on the scenario was
> that these were rejected and reverted because they caused breakage
Your take is not quite correct.
No PR was rejected due to breakage.
There was exactly one
On 2023/02/13 01:58, "G. P. B." wrote:
> We have had completely broken builds for longer days due to some other
> random changes, and we didn't revert them but fixed them as a follow-up.
> We still, for over 6 months now, have a "broken" ASAN build due to phpdbg
> messing up the analyser and
On 2023/02/13 10:28, Dmitry Stogov wrote:
> It's OK when commits are reverted.
> You are working in a common repository, and if your commits become stoppers
> for others they have to be reverted.
> Some of my commits were reverted as well.
That doesn't explain why you demanded to revert
On 2023/02/13 11:05, Dmitry Stogov wrote:
> The RFC proposes merging into the "master" branch.
> And I voted exactly against this.
If not "master", what branch would you prefer?
That's a dumb question, of course, because "master" is where all
future versions branch off, don't they?
Saying "no"
On 2023/02/01 13:13, Max Kellermann wrote:
> Voting starts now, please vote on my RFC:
> https://wiki.php.net/rfc/include_cleanup
Hi,
voting of https://wiki.php.net/rfc/include_cleanup has ended today at
15 UTC.
The majority of voters (52%) voted "Yes" on the primary vote -
On 2023/01/30 11:26, Max Kellermann wrote:
> If nobody objects, I'll announce the start of voting on February 1st.
That's today.
Voting starts now, please vote on my RFC:
https://wiki.php.net/rfc/include_cleanup
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, vi
On 2023/01/30 11:26, Max Kellermann wrote:
> If nobody objects, I'll announce the start of voting on February 1st.
That's today.
Voting starts now, please vote on my RFC:
https://wiki.php.net/rfc/include_cleanup
Original discussion: https://news-web.php.net/php.internals/119272
[Repost
55 matches
Mail list logo
