Skip to content

Replace RuntimeException in Randomizer::nextInt() by RandomException #9305

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 2 commits into from
Aug 15, 2022

Conversation

TimWolla
Copy link
Member

Honestly, this exception likely should not really exist, because it's not really actionable by the user, but in any case it should not be a RuntimeException.

@zeriyoshi Is there a specific reason for this Exception? Should we just clamp a 64 bit value (which is the maximum generated size, even for user engines) to the upper 32 bits for 32 bit systems (which are the only systems that will ever throw the exception)?

@zeriyoshi
Copy link
Contributor

@TimWolla
This exception exists to prevent incompatibility when migrating from a 32-bit to a 64-bit environment.
Since many PHP runtime environments are already 64-bit, this was not considered a problem. For 32-bit environments, this problem can be avoided by using Mersenne Twister or your own RNG implementation.

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.

If this an issue only on 32bit, do we have a specific 32bit test which tests this and throws and exception?

@TimWolla TimWolla force-pushed the random-no-runtimeexception branch from 771bc83 to 73c46b5 Compare August 12, 2022 20:28
@TimWolla
Copy link
Member Author

do we have a specific 32bit test which tests this and throws and exception?

We do now!


@zeriyoshi Okay, that makes sense to me. Then this just needs to change the exception type for proper consistency. Please review 😃

Copy link
Contributor

@zeriyoshi zeriyoshi left a comment

Choose a reason for hiding this comment

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

LGTM
Sorry I'm late!

@TimWolla TimWolla merged commit 3b48a20 into php:master Aug 15, 2022
@TimWolla TimWolla deleted the random-no-runtimeexception branch August 15, 2022 09:07
@TimWolla
Copy link
Member Author

Sorry I'm late!

In time for Beta 3, so that's fine.

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