Skip to content

Simplify parsing of affine matrix arguments #17094

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.

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

Merged
merged 1 commit into from
Dec 10, 2024
Merged

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented Dec 9, 2024

This code repetition is prone to errors and makes the code harder to read than necessary. We simplify at the cost of making parsing of ints slightly less performant (what should not really matter in practice).


So far this is fully backwards compatible. However, given that the $clip argument of imageaffine() uses zend_get_long() without further type checks, we could use zend_get_double() for the matrix arguments also without further type checks. Or perhaps being more strict. As is, the documentation (which is highly incomplete for now) would need to go to great lengths to explain which types are supported for the array elements and how they are handled.

This code repetition is prone to errors and makes the code harder to
read than necessary.  We simplify at the cost of making parsing of ints
slightly less performant (what should not really matter in practice).
Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

MSTM, should we add a zval_try_get_double API?

@cmb69
Copy link
Member Author

cmb69 commented Dec 10, 2024

should we add a zval_try_get_double API?

Sounds like a good idea. It might be also reasonable to somehow merge the implementations of zval_get_long() and zval_try_get_long() etc.; there's quite some code duplication there.

@cmb69 cmb69 merged commit a7785e8 into php:master Dec 10, 2024
10 checks passed
@cmb69 cmb69 deleted the cmb/affine-args branch December 10, 2024 15:09
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