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

Remove ->last_unsafe from php_random_status #9132

Merged
merged 1 commit into from
Jul 26, 2022

Conversation

TimWolla
Copy link
Member

Whenever ->last_unsafe is set to true an exception has been thrown. Thus we
can replace the check for ->last_unsafe with a check for EG(exception)
which is a much more natural way to ommunicate an error up the chain.

Whenever ->last_unsafe is set to `true` an exception has been thrown. Thus we
can replace the check for `->last_unsafe` with a check for `EG(exception)`
which is a much more natural way to ommunicate an error up the chain.
@Danack
Copy link
Contributor

Danack commented Jul 25, 2022

@TimWolla I was about to ask you elsewhere, that having to remember to check the status->last_unsafe seemed prone to mistakes, and is hard to see where the exception is going to be thrown from. So in general I approve.

@@ -301,7 +301,7 @@ PHP_METHOD(Random_Engine_Mt19937, generate)

generated = engine->algo->generate(engine->status);
size = engine->status->last_generated_size;
if (engine->status->last_unsafe) {
if (EG(exception)) {
zend_throw_exception(spl_ce_RuntimeException, "Random number generation failed", 0);
RETURN_THROWS();
Copy link
Contributor

@Danack Danack Jul 25, 2022

Choose a reason for hiding this comment

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

tbh, I don't fully understand this bit, but it looks like an exception has happened elsewhere, and the code is replacing that exception with a new one. Is it possible to set the previous exception, so that the info isn't lost?

And same comment for the other ones below.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it possible to set the previous exception, so that the info isn't lost?

zend_throw_exception is smart enough to do that automatically:

zend_object *previous = EG(exception);

This can be seen in action here:

Error: Failed to generate an acceptable random number in 50 attempts in %s:%d
Stack trace:
#0 %s(%d): Random\Randomizer->shuffleArray(Array)
#1 {main}
Next RuntimeException: Random number generation failed in %s:%d
Stack trace:
#0 %s(%d): Random\Randomizer->shuffleArray(Array)
#1 {main}

I plan to fully remove the RuntimeException here in a future PR, though, so that the underlying error of the broken engine is directly exposed to the user. This PR focuses on the cleanup work to make that possible, one piece at a time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool. TIL zend_throw_exception is smarter than me.

@TimWolla
Copy link
Member Author

I was about to ask you elsewhere, that having to remember to check the status->last_unsafe seemed prone to mistakes

Indeed that's the motivation. And this PR is purely a cleanup without any functional change for the userland developer. #9071 already added exceptions in all places where ->last_unsafe was set, but I wanted to split this in a separate PR, as it's a different logical change.

Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me. And since we haven't fixed the ABI yet, there is no reason to keep .last_unsafe.

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. i was ignorant of the Execute Globals and exception handling.
Thanks!

@TimWolla TimWolla merged commit 5c693c7 into php:master Jul 26, 2022
@TimWolla TimWolla deleted the random-last-unsafe branch July 28, 2022 13:03
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.

5 participants