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 replied to all
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
actu
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 #i
On 20.01.2023 at 13:20, Max Kellermann wrote:
> I think you overestimate the gravity of the changes this RFC will do.
> This isn't a code rewrite, this isn't fragile. If it builds, it's
> okay.
>
> So the worst that can happen is a build breakage with some exotic
> configuration (like the DTrace
Hi
On 1/20/23 11:52, Jakub Zelenka wrote:
As it was said, this is problematic for bug fixes when merging up
and it's extra hurdle for maintainers - read this will slow down the
development.
Can I do anything to help with those merges?
Reduce the diff to absolute minimum to prevent potential
>
>
>> Well the issue with your approach is that 104 PR's is too much to review
> and reviewing that in one go is not good either. So finding some better
> separation and a plan forward could again help you to get this accepted.
>
>
I meant 104 PR's are just too many small chunks to review. That sa
On Fri, Jan 20, 2023 at 12:58 PM Max Kellermann wrote:
> 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 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 instead
On Fri, Jan 20, 2023 at 12:21 PM Max Kellermann wrote:
> 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 w
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 one
On Fri, Jan 20, 2023 at 11:15 AM Max Kellermann wrote:
> 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 o
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/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 gener
On Fri, Jan 20, 2023 at 10:17 AM Max Kellermann wrote:
> On 2023/01/20 10:53, Jakub Zelenka wrote:
>
> > As it was said, this is problematic for bug fixes when merging up
> > and it's extra hurdle for maintainers - read this will slow down the
> > development.
>
> Can I do anything to help with
comments like #include "zend_execute.h" // for zend_vm_calc_used_stack()
Maintaining "the correct" includes manually is hard. Maintaining the
comments manually is even harder.
In
https://github.com/php/php-src/blob/a3a3f326bd32005dd68936edf0e01f98a2fbe163/CODING_STANDARDS.md?plain=1#L269
you
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 thi
On Wed, Jan 18, 2023 at 2:55 PM Max Kellermann wrote:
>
> Here's my RFC: https://wiki.php.net/rfc/include_cleanup
>
>
I think that some logic behind this RFC makes sense but I would see it more
like something for a new project. I'm afraid it's too late for PHP. At
least doing that in one big step
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
> improvements I made so far drown in that n
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, f
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: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
Hi
On 1/18/23 21:37, Flávio Heleno wrote:
This may be a silly question, but in that case, wouldn't #ifdef guards keep
the compiler from including/parsing a.h twice?
It's complicated. Modern compilers may include an optimization for
include guards
(https://gcc.gnu.org/onlinedocs/cppinternals/
On Wed, Jan 18, 2023 at 4:17 PM Tim Düsterhus wrote:
> Hi
>
> On 1/18/23 18:51, Kamil Tekiela wrote:
> > As you said yourself, this refactoring has no practical effect on
>
> It has no practical effect *yet*. The headers need to be untangled first
> before actual optimization can happen.
> > Or m
Hi
On 1/18/23 18:51, Kamil Tekiela wrote:
As you said yourself, this refactoring has no practical effect on
It has no practical effect *yet*. The headers need to be untangled first
before actual optimization can happen.
Or maybe have all
ZEND headers included with a single header?
That wou
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 o
Regards,
On 18 January 2023 16:07:52 GMT, Max Kellermann wrote:
>This argument puzzles me most. I've never heard anybody criticize
>some piece of code for having too MANY code comments, too MUCH
>explanation.
Then I guess you've never heard people quoting Robert "Uncle Bob" Martin, who
is well
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
header
On Mon, 16 Jan 2023, Max Kellermann wrote:
> On 2023/01/16 13:38, Kamil Tekiela wrote:
>
> > What's the impact on other maintainers, especially extension
> > maintainers?
>
> Other maintainers may need to determine which includes they really
> need, instead of relying on everything always alrea
Hi
On 1/18/23 15:55, Max Kellermann wrote:
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 feedback, and try to answer/resolve all questions" - so please
give feedback :-)
Don't forget to add it to the appropria
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 f
On 18.01.2023 at 12:58, Max Kellermann wrote:
> 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!
Done.
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 Wed, 18 Jan 2023 at 09:01, Max Kellermann wrote:
> 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 RF
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 procee
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 file
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
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 g
Hi
On 1/16/23 17:36, Tim Düsterhus wrote:
From my experience contributing to another C project (HAProxy),
cleaning up the the includes can have a measurable impact on build
times. See this commit for example:
Sorry for the duplicate mail, but it just came to my mind checking the
CI build lo
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
Hi
On 1/16/23 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?
From my experience contributing to another C project (HAProxy),
cleaning up the the includes can have a measurable impact on build
tim
Have you done any benchmarking in terms of build time? Is there any
tangible difference or is it only theoretical?
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 be
On Mon, Jan 16, 2023 at 7:54 AM Max Kellermann wrote:
> 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
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 GitHu
On Mon, 16 Jan 2023 at 12:03, Max Kellermann 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
> h
Hi Max,
Thanks for describing the situation.
We do not often vote on implementation changes that don't affect PHP
language itself.
At first, I even considered these changes as "I don't care".
However, they are turned up into huge patches that make troubles for PHP
maintenance.
As we didn't come t
Hi,
Did you create an RFC already? Or is this RFC-like discussion?
In either case, if you are asking community to come to a decision, we need
to see some background. Why do you want to change this? What's the benefit?
What's the impact on other maintainers, especially extension maintainers?
Do you
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 #inc
48 matches
Mail list logo
