Skip to content

Conversation

@TimWolla
Copy link
Member

@TimWolla TimWolla commented Oct 4, 2025

This macro was hiding control flow (the return statement) and thus was particularly unhygienic.

This macro was hiding control flow (the return statement) and thus was
particularly unhygienic.
@TimWolla
Copy link
Member Author

TimWolla commented Oct 5, 2025

@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 😄

Copy link
Member

@bwoebi bwoebi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should do that.
This macro is simple enough and properly abstracts what it does.

I don't mind a rename/alias to RETURN_WRONG_PARAM_COUNT(); or similar, if you want to indicate the control flow as part of the macro.

@TimWolla
Copy link
Member Author

This macro is simple enough and properly abstracts what it does.

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.

@TimWolla TimWolla removed the request for review from devnexen November 6, 2025 10:24
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.

3 participants