Re: [PHP-DEV] RFC: rules for #include directives

[Levi Morrison via internals]

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 patch (I haven't reviewed that PR carefully, maybe
there was some carelessness but if it's not tested, it's hard to blame
anyone).

Lastly, I do not care for the comments which explain why a header is
included. My experience is that these comments bitrot quite quickly.

Overall, I hope we can resume this effort.

-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php

Re: [PHP-DEV] RFC: rules for #include directives

[Max Kellermann]

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 guess CI
builds in the cloud aren't very useful for measuring build times.

My measurements with "perf stat" on a (bare-metal) build machine that
was idle did not produce a meaningful difference.  The noise was
smaller, but still too much to see a real difference.

-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php

Re: [PHP-DEV] RFC: rules for #include directives

[Tim Düsterhus]

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 logs from before and after the revert:


-

Before (x64 Release ZTS)

https://github.com/php/php-src/actions/runs/3924250312/jobs/6708385711

After:

https://github.com/php/php-src/actions/runs/3929927482/jobs/6719316931

9:26 minutes -> 9:52 minutes

Before (x64 Debug NTS):

https://github.com/php/php-src/actions/runs/3924250312/jobs/6708385677

After:

https://github.com/php/php-src/actions/runs/3929927482/jobs/6719316800

4:14 minutes -> 5:11 minutes

-

So with the changes that already happened, Max managed to shave off 30 
seconds (5%) for the x64 Release ZTS build and 1 minute (19%) for the 
x64 Debug NTS build. This difference would likely become even larger, as 
to my understanding the cleanup wasn't complete yet.


Best regards
Tim Düsterhus

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php

Re: [PHP-DEV] RFC: rules for #include directives

[Max Kellermann]

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
included everywhere, which in turn include too much.  The many small
improvements I made so far drown in that noise.

Once I have disentangled and splitted those fat headers (e.g. move
zend_result to a separate header to avoid including zend_types.h
everywhere), the speedup will be measurable and perceptible.  It's too
early to prove it with numbers.

-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php

Re: [PHP-DEV] RFC: rules for #include directives

[Tim Düsterhus]

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 
times. See this commit for example:


https://github.com/haproxy/haproxy/commit/340ef2502eae2a37781e460d3590982c0e437fbd

They go even as far as regularly reordering object files by build time 
to optimally keep multi-core machines busy to shave off the last few 
milliseconds:


https://github.com/haproxy/haproxy/commit/d2ff5dc3ebba1163749e2d874cce5892570f540a

Even for incremental build having a hypothetical 100ms decrease in build 
time can make the difference between "this is fast enough to wait" and 
"this is so slow that my mind wanders around while waiting to build". 
And in CI which doesn't do incremental builds we won't needlessly burn 
CPU time that is better spent elsewhere.


Thus I'm in favor of this cleanup.

Best regards
Tim Düsterhus

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php

Re: [PHP-DEV] RFC: rules for #include directives

[Kamil Tekiela]

Have you done any benchmarking in terms of build time? Is there any
tangible difference or is it only theoretical?

Re: [PHP-DEV] RFC: rules for #include directives

[Max Kellermann]

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 faster, because the C compiler has fewer
includes (= less source code) to process in each compilation unit.

-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php

Re: [PHP-DEV] RFC: rules for #include directives

[Chase Peeler]

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 this process - so far, I've only contributed PHP
> code through GitHub, and I was surprised by today's sudden resistance
> against my code cleanup by two PHP developers, after the others
> welcomed by changes.)
>
> > 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?
>
> For cleaner code, more readable code (less obscurity in huge amounts
> of undocumented #includes), more correct code, less fragile code,
> reduced compile times.
>
> > 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 already there by
> including one random zend_* header.
>
> Extension maintainers may see build failures because, for example,
> they forgot to include errno.h but used "errno".  This previously
> worked because all PHP headers included errno.h even though there was
> no reason to.  These build failures are bugs in those extensions, and
> revealing them is a good thing, even though it seems tedious at first.
>
> > Do you see any downsides to your new approach?
>
> Like any code cleanup, this can result in regressions for parts of PHP
> that are not covered by a test and not built with the CI, like the
> DTrace regression yesterday.
>
> And code cleanups can easily reveal existing bugs, which is a good
> thing, but those bugs can be hidden at first - e.g. failure to
> explicitly include "php_config.h" will (silently) disable features and
> break things.  That may cause some temporary pain, but in the end will
> result in better code, though some will believe that it isn't a worthy
> goal or not worth the temporary pain.
>
> Derick Rethans wrote my idea "adds nothing but clutter", but I guess
> he should explain what bothers him; I don't comprehend this, because
> from my perspective, I intend to remove clutter.
>
> Dmitry Stogov said it's "just a useless permutation", but I can't
> understand this argument either.
>
> Max
>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: https://www.php.net/unsub.php
>
>
For those of us that don't know the finer details of the build process,
will that have any impact on the final product such as reduced binary sizes
or better performance? I'm not saying that code cleanup isn't a good enough
reason to do this on its own, just curious if there are other advantages
beyond that.
-- 
Chase Peeler
chasepee...@gmail.com

[PHP-DEV] Re: [PHP_DEV] [RFC] Add file_descriptor() function

[Christoph M. Becker]

On 16.01.2023 at 16:01, G. P. B. wrote:

> I would like to start the discussion about the "Add file_descriptor()
> function" RFC:
> https://wiki.php.net/rfc/file-descriptor-function
>
> This RFC proposes the addition of the file_descriptor() function to
> retrieve the underlying file descriptor of stream if it exists. This is
> useful when interacting with a USB device.

Thanks for the RFC!

I wonder, though, what happens if the file descriptor is then
manipulated directly, but the stream is accessed later.  Probably,
there's no way to keep the stream resource properly working in all cases.

Isn't the DIO extension[1] already sufficient for such kind of low-level
stuff?

[1] 

--
Christoph M. Becker

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php

Re: [PHP-DEV] [RFC] Add SameSite cookie attribute parameter

[Christian Schneider]

Am 16.01.2023 um 14:39 schrieb G. P. B. :
> On Sun, 15 Jan 2023 at 20:58, Christian Schneider  
> wrote:
> Some comments:
> - I am not convinced that we should introduce a third way of providing 
> parameters to setcookie(). I don't think this function is used often enough 
> in common code to add yet another iteration of the API. Assuming there is 1 
> to 2 places in your framework using this I don't think many bugs will go 
> unnoticed. Adding a warning to illegal 'samesite' values in $options would 
> IMHO be enough if stricter checking is wished for.
> 
> How is this providing a third way of providing parameters to setcookie()? As 
> it is, if I want to use named arguments and the typed parameters, I cannot 
> set the SameSite attribute.
> I've heard multiple people waste time due to a SameSite bug because they made 
> a typo, and it took them way too long to figure out the issue as they thought 
> they had a server configuration problem.
> Adding a warning for invalid values is effectively turning that option into 
> an Enum, which at this point one might as well have a proper Enum.

Maybe third API was misleading, what I mean is that we had the (old) way of 
positional arguments for options which did not include something like samesite 
and the (new) way with the options array. So I would assume most code was 
migrated to $options.
Now you are suggesting of updating the old API with an additional positional 
argument which would mean changing the array-form back to positional.
Is your plan to deprecate the $options-API at some point in the future? Having 
two APIs offering the same functionality seems a bit confusing to me.

Alternatively the 'samesite' option in $options could accept SameSite::Lax. 
This would accomplish (almost) the same with a smaller change. Disclosure: I 
like arrays for options as they allow for array addition etc. Something similar 
can be done with ...$options, true :-)

Side-Note: Isn't SameSite a very generic name in the global namespace? I'm not 
sure what the PHP policy is here.

>   - I don't like the camelCase of $sameSite as this is different from all the 
> other parameters, e.g. $expires_or_options (yes, this is a pseudo-parameter 
> name, I know) and $httponly. Looking at a couple of functions in the standard 
> PHP set I didn't see any $camelCase.
> 
> ACK, it should probably be in snake case and be $same_site

Looking at $httponly I would have expected $samesite.

>   - A more generic question: How are Enums handled concerning future 
> additions of values vs. BC compatibility? What is the migration plan there if 
> one wants to support both old and new PHP versions?
> 
> The parameter is optional, so I don't understand what migration plan you need?
> If in the, unlikely, event that a new value is added this should be added as 
> a case in the enum in the next minor version of PHP.
> In the, again unlikely, event that a value is deprecated, the corresponding 
> enum case should also be deprecated following the normal PHP deprecation 
> process.
> 
> I only decided to make this an enum because I deem it very unlikely for a new 
> valid attribute value to be introduced, the new IETF RFC clarifies and amends 
> the behaviour of the 3 valid attribute values that have always been the same 
> since 2016.

I was talking about extending Enums, not the parameter and not SameSite 
specifically.
Are there any Enums in core PHP APIs where new values could be added in the 
future and how is the plan for code supporting multiple PHP version there? This 
was just something which crossed my mind when thinking about APIs with Enums. 
This is not really related to this RFC so I understand if you want to ignore 
this part :-)

Regards,
- Chris

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php

Re: [PHP-DEV] [PHP_DEV] [RFC] Add file_descriptor() function

[Bob Weinand]

> Am 16.01.2023 um 16:01 schrieb G. P. B. :
> 
> Hello internals,
> 
> I would like to start the discussion about the "Add file_descriptor()
> function" RFC:
> https://wiki.php.net/rfc/file-descriptor-function
> 
> This RFC proposes the addition of the file_descriptor() function to
> retrieve the underlying file descriptor of stream if it exists. This is
> useful when interacting with a USB device.
> 
> Best regards,
> 
> George P. Banyard

Hey George,

I like the idea. However two questions:

Why is it its own function and not part of stream_get_meta_data()? (which you 
anyway want to check to ensure that it's the proper stream type)
Why are all resources throwing a TypeError and not only resources which aren't 
streams? I could imagine calling that function to check whether the stream is 
in memory or backed by a fd for example. I'd propose to return a proper 
checkable value like -1 in that case.

Thanks,
Bob

[PHP-DEV] [PHP_DEV] [RFC] Add file_descriptor() function

[G. P. B.]

Hello internals,

I would like to start the discussion about the "Add file_descriptor()
function" RFC:
https://wiki.php.net/rfc/file-descriptor-function

This RFC proposes the addition of the file_descriptor() function to
retrieve the underlying file descriptor of stream if it exists. This is
useful when interacting with a USB device.

Best regards,

George P. Banyard

Re: [PHP-DEV] [RFC] Add SameSite cookie attribute parameter

[G. P. B.]

On Sun, 15 Jan 2023 at 16:40, Nicolas Grekas 
wrote:

> Hi George,
>
> There's quite some activity on the HTTP cookies side.
> I read about SameParty and Partitioned attributes recently, see:
> - https://developer.chrome.com/docs/privacy-sandbox/chips/
> - https://github.com/cfredric/sameparty
>
> Maybe we should have a plan that works for these too?
>
> Nicolas
>

Considering those cookie attributes are still in an experimental design
phase and are not universally agreed upon by user agents, I wouldn't want
to add a specific parameter for them.
However, a more generic solution by overloading the options array to
accepts arbitrary key:value pairs might be something to pursue, but this is
out of scope for this RFC as just adding this to the current option array
forces people to use that signature which is less type safe.


On Sun, 15 Jan 2023 at 20:58, Christian Schneider 
wrote:

> Some comments:
> - I am not convinced that we should introduce a third way of providing
> parameters to setcookie(). I don't think this function is used often enough
> in common code to add yet another iteration of the API. Assuming there is 1
> to 2 places in your framework using this I don't think many bugs will go
> unnoticed. Adding a warning to illegal 'samesite' values in $options would
> IMHO be enough if stricter checking is wished for.
>

How is this providing a third way of providing parameters to setcookie()?
As it is, if I want to use named arguments and the typed parameters, I
cannot set the SameSite attribute.
I've heard multiple people waste time due to a SameSite bug because they
made a typo, and it took them way too long to figure out the issue as they
thought they had a server configuration problem.
Adding a warning for invalid values is effectively turning that option into
an Enum, which at this point one might as well have a proper Enum.


> - I don't like the camelCase of $sameSite as this is different from all
> the other parameters, e.g. $expires_or_options (yes, this is a
> pseudo-parameter name, I know) and $httponly. Looking at a couple of
> functions in the standard PHP set I didn't see any $camelCase.
>

ACK, it should probably be in snake case and be $same_site


> - A more generic question: How are Enums handled concerning future
> additions of values vs. BC compatibility? What is the migration plan there
> if one wants to support both old and new PHP versions?
>

The parameter is optional, so I don't understand what migration plan you
need?
If in the, unlikely, event that a new value is added this should be added
as a case in the enum in the next minor version of PHP.
In the, again unlikely, event that a value is deprecated, the corresponding
enum case should also be deprecated following the normal PHP deprecation
process.

I only decided to make this an enum because I deem it very unlikely for a
new valid attribute value to be introduced, the new IETF RFC clarifies and
amends the behaviour of the 3 valid attribute values that have always been
the same since 2016.


> Regards,
> - Chris


On Sun, 15 Jan 2023 at 21:08, Claude Pache  wrote:

> Hi,
>
> Some technical remarks:
> * The new parameter name should be `$samesite` (all lowercase), in order
> to match with the casing of the corresponding key in `$options`.
>

I don't agree with this, the name $samesite all lowercase is far from
optimal, however it should be in snake case ($same_site) in accordance with
the other parameters.


> * I think that you should add the case `SameSite::Omit` (which is the
> current default). This is not only for BC, but also for FC if, for some
> reason, `SameSite: Lax` is replaced by some attribute that supersedes it.
> Or if `SameSite: Lax` becomes the default, and therefore redundant. —
> Having `SameSite::Omit` instead of `null` would mean that it would be
> difficult to miss it by accident.
>

Same idea as in my reply to Chris, it is extremely unlikely that a new
attribute value will be added, moreover the point of using SameSite::Lax as
the default is that it will become the default (it already is in Chrome,
and is gated between a flag in Firefox) as currently in any case you *must*
provide a SameSite values for your cookies as the behaviour end-users are
going to have depends on their browser (e.g. Chrome defaulting to Lax,
while Firefox and Safari still default to None currently) and thus omitting
the value is, IMHO, a bug waiting to happen.


> That said, I am much more interested in being able to add custom cookie
> attributes. Back when SameSite was introduced (on the web, not in PHP), I
> recall that I had to use some hack in order to include them in my session
> cookie (before upgrading to PHP 7.3). The new cookie attributes mentioned
> by Nicolas in the other mail are probably too experimental in order to
> support them officially, but it could be interesting to be able to include
> them nonetheless, e.g. using some `customAttributes` parameter.
>

As said to Nicolas, this is definitely interesting, but 

Re: [PHP-DEV] RFC: rules for #include directives

[Max Kellermann]

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 GitHub, and I was surprised by today's sudden resistance
against my code cleanup by two PHP developers, after the others
welcomed by changes.)

> 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?

For cleaner code, more readable code (less obscurity in huge amounts
of undocumented #includes), more correct code, less fragile code,
reduced compile times.

> 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 already there by
including one random zend_* header.

Extension maintainers may see build failures because, for example,
they forgot to include errno.h but used "errno".  This previously
worked because all PHP headers included errno.h even though there was
no reason to.  These build failures are bugs in those extensions, and
revealing them is a good thing, even though it seems tedious at first.

> Do you see any downsides to your new approach?

Like any code cleanup, this can result in regressions for parts of PHP
that are not covered by a test and not built with the CI, like the
DTrace regression yesterday.

And code cleanups can easily reveal existing bugs, which is a good
thing, but those bugs can be hidden at first - e.g. failure to
explicitly include "php_config.h" will (silently) disable features and
break things.  That may cause some temporary pain, but in the end will
result in better code, though some will believe that it isn't a worthy
goal or not worth the temporary pain.

Derick Rethans wrote my idea "adds nothing but clutter", but I guess
he should explain what bothers him; I don't comprehend this, because
from my perspective, I intend to remove clutter.

Dmitry Stogov said it's "just a useless permutation", but I can't
understand this argument either.

Max

-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php

Re: [PHP-DEV] RFC: rules for #include directives

[G. P. B.]

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
>  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

Re: [PHP-DEV] RFC: rules for #include directives

[Dmitry Stogov]

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 to an agreement, the best decision should be done by the
PHP community.

In the current state of the implementation I'm personally against this, but
I might change my opinion if this is targeted to PHP-9 and the
implementation is done in a some better way.

Thanks. Dmitry.


On Mon, Jan 16, 2023 at 3:03 PM 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
>  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
>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: https://www.php.net/unsub.php
>
>

Re: [PHP-DEV] RFC: rules for #include directives

[Kamil Tekiela]

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 see any downsides to your new approach?

Regards,
Kamil

[PHP-DEV] RFC: rules for #include directives

[Max Kellermann]

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

-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php