Skip to content

Remove FAST_ZPP macro and plain zpp fallback code #2118

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

Closed

Conversation

hikari-no-yume
Copy link
Contributor

@hikari-no-yume hikari-no-yume commented Sep 5, 2016

This gets rid of the FAST_ZPP macro, and all the dead code (which sometimes fails to compile when necromanced, or otherwise has tests fail) conditionally compiled when it is not defined (i.e. never actually compiled). It puts Fast ZPP on an equal footing with the traditional zend_parse_parameters API, rather than treating it as a provisional measure that might be removed at any time (which, given it has an RFC that passed, and given that it's in the 7.0, 7.1 and master branches, is unlikely).

Without the requirement to write fallback code, extension authors can choose to use the more performant and more readable Fast ZPP API exclusively, if they so choose, and PHP will stop accumulating dead code segments which can easily go out of sync with their alive relatives (and indeed, have done so).

This targets 7.0 because it is the earliest release which contains it, and we can remove it from 7.0 and all subsequent versions safely: the code this patch removes is dead and the option of undefining the FAST_ZPP macro can't possibly have been in use by anyone (as evidenced by the fact that every stable release of PHP 7 will fail to compile if you do so).

The idea of removing FAST_ZPP and the conditionally-compiled code segments containing old zpp code has been brought up on internals before, and has received no objections.

@hikari-no-yume
Copy link
Contributor Author

@weltling what do you feel about this?

@weltling
Copy link
Contributor

@TazeTSchnitzel actually, fast ZPP is always enabled in the last 10 stable 7.0 releases. IMO it has proven itself. For the core, removing the fallbacks clauses seems ok. However, the FAST_ZPP macro define needs to be kept in 7.0 and 7.1 at least. It is exported in a public header and thus can affect already released non core extensions.

Thanks.

@hikari-no-yume
Copy link
Contributor Author

hikari-no-yume commented Sep 11, 2016

@weltling Core hasn't been able to build without it for ages, but I understand the concern with other extensions. Would just keeping around a single #define FAST_ZPP 1 work?

@hikari-no-yume
Copy link
Contributor Author

Added back the FAST_ZPP macro with an explanatory comment. Was silly of me to remove it without thinking about external extensions, I should have thought of that.

This should be okay, I assume?

@weltling
Copy link
Contributor

@TazeTSchnitzel yeah, the define is what I meant. AFM no issues otherwise.

Thanks.

@hikari-no-yume
Copy link
Contributor Author

Great. Do you think I should merge?

@KalleZ
Copy link
Member

KalleZ commented Sep 11, 2016

Go for it :)

@hikari-no-yume
Copy link
Contributor Author

Merged as d690014.

@hikari-no-yume
Copy link
Contributor Author

Additional zpp fallback code which only exists in master has been removed with an additional commit here: 3cc9090.

@hikari-no-yume
Copy link
Contributor Author

Is it worth noting in NEWS, do you think?

@KalleZ
Copy link
Member

KalleZ commented Sep 12, 2016

I don't think its NEWS worthy, in general I don't think internal API changes, if anything it should be in UPGRADING.INTERNALS imo

@weltling
Copy link
Contributor

Thanks for merging, Andrea. Since there's nothing changed to outside, probably there's nothing to mention in the NEWS.

thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants