Skip to content

Change return values to bool or void in FPM #9911

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

Draft
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Nov 9, 2022

A lot of functions in FPM declare an int return type but only return one, or two distinct values.
Since C99 bool is defined by the standard and I have thus changed the 0/-1 return values to true and false, however to not break API usage (e.g. to make merging upwards) it would be possible to return SUCCESS and FAILURE and mark the return type as zend_result.

This came about as I was checking that a doc PR for FPM was correct and the conditionals 0 > confused me at first, and I think using the more standard boolean behaviour (or changing to checking explicitly for == FAILURE) is clearer.

Either the return value was always the same, or just returned either 0 or -1
Either the return value was always the same, or just returned either 0 or -1
The return value was always either 0 or -1
The return value was always either 0 or -1
The return value was always one of two values.
Either the return value was always the same, or just returned either 0 or -1
The return value was always one of two values.
The return value was always one of two values.
The return value was always either 0 or -1
The return value was always either 0 or -1
Either the return value was always the same, or just returned either 0 or -1
The return value was always either 0 or -1
The return value was always either 0 or -1
The return value was always either 0 or -1
The return value was always either 0 or -1
@bukka
Copy link
Member

bukka commented Nov 10, 2022

I'm sorry but this is too invasive and will make fixing bugs really painful when merging up. I will be happy to reconsider it once most of the FPM bugs are fixed but not at this stage I'm afraid as the value of this is more cosmetic really...

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

Successfully merging this pull request may close these issues.

2 participants