Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Some miscellaneous ext/standard cleanup #15953

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Some miscellaneous ext/standard cleanup #15953

wants to merge 7 commits into from
+69 −241

Conversation

DanielEScherzer
Copy link
Contributor

  • Should overall have no real changes
  • Each commit/file can be reviewed independently

@DanielEScherzer
ext/standard/array.c: remove some unreachable code
0efe475 
- If a `case` block always returns something, a `break` is unneeded

- If every `case` block returns something, and there is a `default` block, then
any code after the `switch` is unreachable, and can be replaced with
`ZEND_UNREACHABLE()`
@DanielEScherzer
ext/standard/exec.c: combine conditions, update docs
d31b24f 
While `php_escape_shell_cmd()` did indeed `emalloc` a string that needed to be
freed by the caller in the original implementation that was put in GitHub
(see commit 257de2b) a few months ago the
return type was changed to use `zend_string`, see php#14353.
@DanielEScherzer
ext/standard/html.c: clean up a bit
56b79ad 
- Instead of manually wrapping `assert()` statements with a `ZEND_DEBUG`
condition, use `ZEND_ASSERT()`, so that for non-debug builds the compiler is
told to assume that the assertion is correct.

- Update some return types for methods that return `zend_result`.
@DanielEScherzer
ext/standard/iptc.c: drop some unused return values
511cc45
@DanielEScherzer
ext/standard/mail.c: cleanup a bit
52daa74 
- `php_mail_build_headers_check_field_name()` returns `zend_result`, not bool

- use `EMPTY_SWITCH_DEFAULT_CASE()` and `ZEND_UNREACHABLE()` for impossible
conditions
@DanielEScherzer
ext/standard/math.c: add MATH_PHP_FUNCTION_WRAPPER() for deduplication
440ff2a 
17 different functions are implemented in essentially the same way - the single
parameter (a `double`) is parser, a math function written in C (named the same
as the PHP function being implemented) is called, and the result is returned
with `RETURN_DOUBLE()`. Add a helper macro to deduplicate the implementations.
@DanielEScherzer
ext/standard/proc_open.c: inline init_process_info()
8b547cb 
Single-use one-line method that is not exported, no need to exist separately
@DanielEScherzer
Copy link
Contributor Author

CC @Girgias - the exec.c documentation updates are a follow-up to your #14353

@nielsdos
Copy link
Member

I don't like the changes done with the switches on the enums: having an exception thrown instead of causing UB seems more defensive to me.

@DanielEScherzer
Copy link
Contributor Author

I don't like the changes done with the switches on the enums: having an exception thrown instead of causing UB seems more defensive to me.

Which file is this about? The ZEND_UNREACHABLE(); that I used replaced a return statement, so isn't a non-void function without a return at the end going to cause undefined behavior anyway?

@iluuu1994
Copy link
Member

@DanielEScherzer Thank you for your PR! It would be easier if you created a PR for each unrelated change, as then they can be merged individually. It'll also be easier to keep the history clean, as you can simply commit to the branch (even via GUI) and we can just squash when merging.

so isn't a non-void function without a return at the end going to cause undefined behavior anyway?

Each path must return a value. Whether there's a return at the very end, or a branch with both paths returning doesn't matter. In reality we can just remove the ZEND_UNREACHABLE();. Omitting it actually has the benefit of making the compiler check whether the end of the function is reachable through some path. I.e. this will warn:

switch (sort_type & ~PHP_SORT_FLAG_CASE) {
	case PHP_SORT_NUMERIC:
		if (reverse) {
			return php_array_reverse_data_compare_numeric;
		} else {
			// Oops
			// return php_array_data_compare_numeric;
		}
	case PHP_SORT_STRING:

/home/ilutov/Developer/php-src/ext/standard/array.c: In function ‘php_get_data_compare_func’:
/home/ilutov/Developer/php-src/ext/standard/array.c:494:1: warning: control reaches end of non-void function [-Wreturn-type]

@nielsdos
Copy link
Member

@DanielEScherzer I was specifically referring to changes like in php_mail_build_headers_elem in this case, but maybe that's not a big deal. I also saw in another PR IIRC such a change for a PHP userland enum but I could be wrong.

@Girgias
Copy link
Member

Girgias commented Sep 20, 2024

Other than the mail.c changes which I was kinda improving in #15379 (which if you want to take over I'm happyt to let you do :) ) this looks good to me and can be merged as a rebase.

So probably drop that commit and we can proceed with the merge.

Also for your information, I added some tasks on our "meta" issue tracker: https://github.com/php/php-tasks/issues

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bukka bukka Awaiting requested review from bukka bukka is a code owner

Assignees
No one assigned
Labels
Extension: standard
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@DanielEScherzer @nielsdos @iluuu1994 @Girgias