Skip to content

Implement GMP::__construct - #10225

Closed
ndossche wants to merge 1 commit into
php:PHP-8.2from
ndossche:gmp-constructor
Closed

Implement GMP::__construct#10225
ndossche wants to merge 1 commit into
php:PHP-8.2from
ndossche:gmp-constructor

Conversation

@ndossche

@ndosschendossche commented Jan 4, 2023

Copy link
Copy Markdown
Member

Supersedes GH-10158
FixesGH-10155

Implements a proper constructor for GMP as discussed in both GH-10158 and https://externals.io/message/119216.
Targeting 8.2.x as proposed on the mailing list thread.
cc @Girgias

Not sure if I did everything correctly, looking forward for reviews :)

@ndosschendossche mentioned this pull request Jan 4, 2023
Closed
Girgias
Girgias approved these changes Jan 5, 2023
View reviewed changes

@GirgiasGirgias left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Minor nits but LGTM otherwise

Comment threadext/gmp/gmp.c Outdated
Comment threadext/gmp/gmp.c Outdated
Comment threadext/gmp/gmp.c Outdated
Comment threadext/gmp/gmp.c Outdated
Comment threadext/gmp/tests/construct.phpt Outdated

@GirgiasGirgiasJan 5, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just for the sake of completeness, add a test with a non-numeric string as the number (e.g. 'hello', or '')

@ndossche

ndossche commented Jan 5, 2023

Copy link
Copy Markdown
MemberAuthor

Thanks for the review, I updated the patch with your suggestions.

@ndossche

ndossche commented Jan 5, 2023

Copy link
Copy Markdown
MemberAuthor

I checked the failures, they are unrelated to the code changes.

@TimWollaTimWolla mentioned this pull request Jan 6, 2023
Closed
@TimWolla
TimWolla requested a review from GirgiasJanuary 17, 2023 14:52
@Girgias

Girgias commented Jan 17, 2023

Copy link
Copy Markdown
Member

@ramsey@saundefined@adoy What do you think about landing this in 8.2?

@adoy

adoy commented Jan 17, 2023

Copy link
Copy Markdown
Member

I see no issue adding this to 8.2.

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

Reviewers

@adoyadoyadoy approved these changes
@GirgiasGirgiasAwaiting requested review from GirgiasGirgias is a code owner

Assignees

No one assigned

Labels

Extension: gmp

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ndossche@Girgias@adoy