-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Replace @deprecated with #[\Deprecated] for internal constants
#18780
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace @deprecated with #[\Deprecated] for internal constants
#18780
Conversation
550eb12 to
2792802
Compare
If you look at the filter arginfo you can see that the same strings are allocated twice. Do you think you can avoid this at least within the same arginfo file?
If you look at the filter arginfo you can see that the same strings are allocated twice. Do you think you can avoid this at least within the same arginfo file?
Not easily - I'll add it to my list of improvements for gen_stub.php, but this isn't anything new
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok for ext/random
the same strings are allocated twice
It should probably be safe to just intern them all. They are always allocated on start-up, so the memory will be used either way. Interning should just reduce it, no?
the same strings are allocated twice
It should probably be safe to just intern them all. They are always allocated on start-up, so the memory will be used either way. Interning should just reduce it, no?
Probably, and I will definitely look into this, but I'd prefer not to block the rest of the patch
the same strings are allocated twice
It should probably be safe to just intern them all. They are always allocated on start-up, so the memory will be used either way. Interning should just reduce it, no?
Yes I agree.
If you pinky promise that you're gonna look at it before 8.5's release I'll allow it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK for DOM, Enchant, SOAP
the same strings are allocated twice
It should probably be safe to just intern them all. They are always allocated on start-up, so the memory will be used either way. Interning should just reduce it, no?
Yes I agree. If you pinky promise that you're gonna look at it before 8.5's release I'll allow it.
Pinky promise - I have #18665 and a few other bits of cleanup in mind that shouldn't change any of the generated arginfo files, and then I'll address the improvements that will affect arginfo (like adding interned strings)
Code owner approvals:
- curl - adoy
- date - derickr
- dom - nielsdos
- enchant - No owner or current maintainer (approved by neilsdos)
- filter - No owner or current maintainer
- mysqli - bukka / kamil-tekiela (approved by bukka)
- pgsql - devnexen
- random - timwolla / zeriyoshi (approved by tim wolla)
- reflection - me
- soap - neilsdos
- standard - bukka
Barring any further review I plan to merge this in the next few days
Only covers constants declared via stub files, others will be handled separately in a later commit. Does not include the intl extension, since that had some errors relating to the cpp code; that extension will be updated separately. Also fix an undefined variable warning in gen_stub.php.
This is already fixed differently in master.
e1037b1 to
f7cfe2a
Compare
Uh oh!
There was an error while loading. Please reload this page.
Only covers constants declared via stub files, others will be handled separately in a later commit.
Does not include the intl extension, since that had some errors relating to the cpp code; that extension will be updated separately.
Also fix an undefined variable warning in gen_stub.php.