Skip to content

[RFC] ext/bcmath: Added bcdivmod #15740

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
Merged
merged 4 commits into from
Sep 22, 2024
Merged

[RFC] ext/bcmath: Added bcdivmod #15740

merged 4 commits into from
Sep 22, 2024

Conversation

SakiTakamachi
Copy link
Member

@SakiTakamachi SakiTakamachi force-pushed the rfc/add_bcdivmod branch 2 times, most recently from a0f6c5d to 26e9759 Compare September 4, 2024 03:24
@SakiTakamachi SakiTakamachi force-pushed the rfc/add_bcdivmod branch 2 times, most recently from dca598e to 24949ec Compare September 4, 2024 06:46
@SakiTakamachi SakiTakamachi force-pushed the rfc/add_bcdivmod branch 2 times, most recently from 4f0eb7f to 697baa1 Compare September 5, 2024 05:26
@SakiTakamachi
Copy link
Member Author

This didn't make it into beta5🙏

@SakiTakamachi SakiTakamachi force-pushed the rfc/add_bcdivmod branch 2 times, most recently from d116e8b to 8d0ecb8 Compare September 19, 2024 13:00
@SakiTakamachi
Copy link
Member Author

By optimizing bcdiv processing, more efficient calculations can be made, but the changes will be quite complex, and we have decided that it will not be possible in time for RC1, so decided to make this change in 8.4 as this.

@SakiTakamachi SakiTakamachi marked this pull request as ready for review September 19, 2024 14:34
nielsdos
nielsdos requested changes Sep 19, 2024
View reviewed changes
ext/bcmath/bcmath.c Outdated

if (scale_param_is_null) {
scale = BCG(bc_precision);
} else if (scale_param < 0 || scale_param > INT_MAX) {
Copy link
Member

Choose a reason for hiding this comment

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

We can use the recently-added helper function for this check now.

ext/bcmath/bcmath.c Outdated
goto cleanup;
}

array_init(return_value);
Copy link
Member

Choose a reason for hiding this comment

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

You're creating a new pair of zvals here. We have a very optimized API to do this as fast as possible: zend_new_pair.

ext/bcmath/bcmath.c Outdated

BC_ARENA_SETUP;

bc_init_num(&quot);
Copy link
Member

Choose a reason for hiding this comment

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

Why do you initialize quot and rem like this instead of setting them to NULL at the declaration site?

ext/bcmath/bcmath.c Outdated
bcmath_number_obj_t *quot_intern = bcmath_number_new_obj(quot, 0);
bcmath_number_obj_t *rem_intern = bcmath_number_new_obj(rem, scale);

array_init(return_value);
Copy link
Member

Choose a reason for hiding this comment

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

Same remark of zend_new_pair.

Girgias
Girgias reviewed Sep 20, 2024
View reviewed changes
@SakiTakamachi
Copy link
Member Author

@nielsdos
Thanks, I fixed these!

@nielsdos
Copy link
Member

You forgot the zend_new_pair call for one of the functions

@SakiTakamachi
Copy link
Member Author

Oh thanks, I'll fix it!

@SakiTakamachi
Copy link
Member Author

@nielsdos
done :)

@SakiTakamachi SakiTakamachi merged commit f6db576 into php:master Sep 22, 2024
1 of 2 checks passed
@SakiTakamachi SakiTakamachi deleted the rfc/add_bcdivmod branch September 22, 2024 21:43
@jrfnl jrfnl mentioned this pull request Oct 5, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Girgias Girgias Girgias left review comments

@nielsdos nielsdos nielsdos approved these changes

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

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

@iluuu1994 iluuu1994 Awaiting requested review from iluuu1994

Assignees

No one assigned

Labels

Category: Optimizer Extension: bcmath

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@SakiTakamachi @nielsdos @Girgias