tree-wide: Replace ZEND_WRONG_PARAM_COUNT() by its definition
#20066
Conversation
This is a direct alias.
This macro was hiding control flow (the return statement) and thus was particularly unhygienic.
TimWolla
requested review from
Girgias,
bukka,
devnexen and
dstogov
as code owners
ext/gd/gd.c
Outdated
| if (ZEND_NUM_ARGS() < 2 || ZEND_NUM_ARGS() > IMAGE_FILTER_MAX_ARGS) { | ||
| WRONG_PARAM_COUNT; | ||
| zend_wrong_param_count(); | ||
| RETURN_THROWS(); | ||
| } else if (zend_parse_parameters(2, "Ol", &tmp, gd_image_ce, &filtertype) == FAILURE) { | ||
| RETURN_THROWS(); | ||
| } | ||
|
|
||
| if (filtertype >= 0 && filtertype <= IMAGE_FILTER_MAX) { | ||
| filters[filtertype](INTERNAL_FUNCTION_PARAM_PASSTHRU); | ||
| } |
There was a problem hiding this comment.
Do we actually need this? Can't we not just have the else if branch, and then the filters handling the "too many" args case?
|
@Girgias I also considered improving the error messages, but I'm not really familiar with all the extensions and I also wanted to avoid combining the internal cleanup with a behavioral change. Especially when the API needs this function, it is not great in the first place, so I feel improving the error message is a little like polishing a turd. So I just did the mechanical change that allowed to clean up zend_API 😄 |
It feels odd that this specific exception-throwing function (which is already very rarely needed and will be needed even more rarely once Gina's PRs ship) is special-cased by having a macro that implies the return when all other exception-throwing functions don't. |
3 participants
This macro was hiding control flow (the return statement) and thus was particularly unhygienic.