Zend: Remove returns after zend_error_noreturn

[kamil-tekiela]

Taken from the article https://helgesver.re/articles/hidden-comments-in-php-src

[NattyNarwhal]

To be honest, I think we could just get rid of those reimplementation functions? unsetenv/setenv are standard on anything Unix shaped (as in, available on v7), and since we don't care about Windows in FPM, we don't ned to worry about that case.

N.B.: clearenv is not standard; it seems to be from a rejected POSIX proposal seemingly only glibc implemented?

[kamil-tekiela]

To be honest, I think we could just get rid of those reimplementation functions? unsetenv/setenv are standard on anything Unix shaped (as in, available on v7), and since we don't care about Windows in FPM, we don't ned to worry about that case.

N.B.: clearenv is not standard; it seems to be from a rejected POSIX proposal seemingly only glibc implemented?

Ok, well, I don't know anything about FPM. I was just trying to fix a superficial problem. But if these functions should be removed, can you open a separate PR and I will drop the commit from my PR?

[NattyNarwhal]

Ok, well, I don't know anything about FPM. I was just trying to fix a superficial problem. But if these functions should be removed, can you open a separate PR and I will drop the commit from my PR?

Just did so. Also looking at other instances of this, and I think I found a similar "fun" comment related to these functions already...

#if !defined(HAVE_SETENV) || !defined(HAVE_UNSETENV)
        /*  if cgi, or fastcgi and not found in fcgi env
                check the regular environment
                this leaks, but it's only cgi anyway, we'll fix
                it for 5.0
        */              
        len = name_len + (value ? strlen(value) : 0) + sizeof("=") + 2;
        buf = (char *) malloc(len);     
        if (buf == NULL) {
                return getenv(name);            
        }                                               
#endif                  

[TimWolla]

Several of the functions “now” had a constant return value of SUCCESS. Should we voidify them at the same time? This will likely allow removing (dead) checks in callers as well.

I'm also noting they also wrongly use int instead of zend_result as the return type, which would implicitly be cleaned up when voidifying them.

[kamil-tekiela]

Several of the functions “now” had a constant return value of SUCCESS. Should we voidify them at the same time? This will likely allow removing (dead) checks in callers as well.

I'm also noting they also wrongly use int instead of zend_result as the return type, which would implicitly be cleaned up when voidifying them.

Should I do it in this PR or in a separate one?

[TimWolla]

For me adjusting the return type logically belongs to this change / PR. It's already removing return statements, and adjusting the return type is just the final consequence.

[kamil-tekiela]

Several of the functions “now” had a constant return value of SUCCESS. Should we voidify them at the same time? This will likely allow removing (dead) checks in callers as well.

I'm also noting they also wrongly use int instead of zend_result as the return type, which would implicitly be cleaned up when voidifying them.

I changed it to zend_result but I could not vodidify them because of zend_implement_serializable which is the only one that returns FAILURE.

[TimWolla]

but I could not vodidify them because of zend_implement_serializable which is the only one that returns FAILURE.

Ah, I didn't realize that the signature needs to match a function pointer signature. That's unfortunate, but zend_result is an improvement.

[TimWolla]

Actually, looking at how interface_gets_implemented() is used: A FAILURE will just result in a generic zend_error_noreturn(), so voidifying that callback and expecting implementers to zend_error_noreturn() themselves would be reasonable. Changing the int to zend_result is already a breaking change anyway.

[ndossche]

I think these kinds of changes are pointless on its own, unless you also take into account what Tim says wrt voidifying. Right now the value of the PR (except for the zend_result type) is still questionable.
Removing returns after the noreturns is of course technically correct, but may warn on some compilers (which was the reason why some returns were added), and don't create more optimization opportunities.
It's only really good to uncover the cases that Tim points out.

[kamil-tekiela]

Makes sense. Both of you have very good points. I have moved the error inside zend_implement_serializable and made the signature void.

[TimWolla]

LGTM except for Changelog remark, thanks.