Minimal refactoring of CLI SAPI #8519
Conversation
|
I'll review this after work, if you can wait 🙂 |
| @@ -2398,12 +2402,12 @@ static void php_cli_server_startup_workers(void) { | |||
| php_cli_server_worker++) { | |||
| pid_t pid = fork(); | |||
|
|
|||
| if (pid == FAILURE) { | |||
| if (pid < 0) { | |||
|
|
||
| server->is_running = 1; | ||
| out: | ||
| if (retval != SUCCESS) { |
There was a problem hiding this comment.
Personally I like the defensive/whitelist approach of != SUCCESS, and I can't find an objective argument in favor of changing this to == FAILURE. I feel that it's mostly a matter of taste :)
I'm not against this change, but unless we have a clear coding standard regarding this, we risk changing the same lines back and forth between different styles.
There was a problem hiding this comment.
The main reason I do these change is if I see != SUCCESS I don't know if the call can only return zend_result or some other value, as I have already seen that in the code base where it looks like the called function only returns zend_return but does not (e.g. returns 1) so it is more a mental marker for me. But I'm not really attached to the equality.
There was a problem hiding this comment.
I generally agree with @arnaud-lb. I don't prefer one over the other, and I'm happy making one the default. But I fear that unless we do have a standard people keep changing things like this back and forth for no good reason.
| @@ -241,23 +241,23 @@ static const char php_cli_server_css[] = "<style>\n" \ | |||
| /* }}} */ | |||
|
|
|||
| #ifdef PHP_WIN32 | |||
| int php_cli_server_get_system_time(char *buf) { | |||
| static bool php_cli_server_get_system_time(char *buf) { | |||
There was a problem hiding this comment.
I know this isn't your code, but the definition could be shared for all architectures.
|
|
||
| server->is_running = 1; | ||
| out: | ||
| if (retval != SUCCESS) { |
There was a problem hiding this comment.
I generally agree with @arnaud-lb. I don't prefer one over the other, and I'm happy making one the default. But I fear that unless we do have a standard people keep changing things like this back and forth for no good reason.
More specific types, some cleanup and voidifying functions which always return
SUCCESS