Skip to content
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

Thoughts on converting some int|string values to GMP? #6

Closed
afk11 opened this issue Nov 8, 2017 · 2 comments
Closed

Thoughts on converting some int|string values to GMP? #6

afk11 opened this issue Nov 8, 2017 · 2 comments

Comments

@afk11
Copy link
Contributor

afk11 commented Nov 8, 2017

Using int|string is done in a few places, but we can't set a return type of int because large (and valid) tags, etc may be above the machines PHP_INT_MAX. We could use a \GMP instance for these cases.

Not sure what you think about this one, API consumers would have to do it wherever they want to use a tag as a key - which we would have do in some places too (tag to tag name I think, plus some constants will just be integers..)

Let me know if it's too invasive to be worth the parameter / return type hints, otherwise I'll try write this up soon

@sop
Copy link
Owner

sop commented Nov 8, 2017

I've been done some thinking on that matter as well.

I made changes just a while back so that numbers that could be reasonably expected to have large values are represented internally as BigInt, which stores the number as a string. Furthermore i changed return values such that methods that used to return int|string now return integer number strictly as a string.

It doesn't feel right to require parameters as GMP or BigInt or any other object. I think for the fluency of the API we should accept untyped parameter in those places and leave the responsibility to the caller.

One possibility would be to introduce fromGMP, fromInt etc. methods, but would it bring any added value to the API?

@afk11
Copy link
Contributor Author

afk11 commented Nov 15, 2017

That's ok, it's also fine the way it is! Closing this.

@afk11 afk11 closed this as completed Nov 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants