[random] split getInt() with no arguments into nextInt() #9057

zeriyoshi · 2022-07-20T01:47:04Z

The Random Extension 5.x and Random Extension Improvement proposals passed, but with the problem of argument overloading.

#8094

As noted by some, argument overloading is now discouraged and should not be implemented anew.

https://externals.io/message/118163#118269
https://github.com/php/php-src/pull/8094/files#r919693108

cc / @TimWolla @kocsismate
I am sorry for the review you gave us.
I have split the PR and corrected the points you pointed out.

guilliamxavier · 2022-07-20T08:18:16Z

ext/random/tests/03_randomizer/basic.phpt

+            if ($e->getMessage() !== 'Generated value exceeds size of int') {
+                die($engine::class . ': nextInt: failure: {$e->getMesasge()}');
+                throw $e;


before this if, other similar tests (compatibility_user.phpt) first do a
if (\PHP_INT_SIZE >= 8) { throw $e; }

typo in the die: $e->getMesasge() should be $e->getMessage()

bug in the die: single-quotes should be double-quotes, or (maybe better) replace string interpolation with concatenation

this throw is unreachable (after a die)

My brain was shutting down.
I will fix it. I am ashamed. 😭

fixed: d78c647

In the case of nextInt(), the method is checked for exception messages, since conditional branching by PHP_INT_SIZE would completely prevent the method from being tested

OK for the fixes (of my last 3 points),
but I don't understand your last comment (about my first point) 😕 I saw

php-src/ext/random/tests/03_randomizer/compatibility_user.phpt

Lines 45 to 52 in d78c647

} catch (\RuntimeException $e) {

if (\PHP_INT_SIZE >= 8) {

throw $e;

}

if ($e->getMessage() !== 'Generated value exceeds size of int') {

throw $e;

}

}

(2 occurrences in that file) so was wondering if here you should do something like

} catch (\RuntimeException $e) { + if (\PHP_INT_SIZE >= 8) { + die($engine::class . ": nextInt: failure: {$e->getMessage()}"); + } if ($e->getMessage() !== 'Generated value exceeds size of int') { die($engine::class . ": nextInt: failure: {$e->getMessage()}"); } }

? or maybe merge the two ifs into one if ($e->getMessage() !== 'Generated value exceeds size of int' || \PHP_INT_SIZE >= 8) (in both files)? (just questions BTW, not blocking this PR)

Sorry. I was confused again.
This is completely unnecessary and should be removed. If it is a good thing to modify at the same time as this PR, I will do so now. 😭

Sorry. I was confused again.

No big deal 😄

This is completely unnecessary and should be removed.

Well, I thought that it was at least testing that nextInt() doesn't throw a "Generated value exceeds size of int" exception on 64-bit systems, but you know better 😉

fixed: 7c41dd9

Fine (I guess, given

php-src/ext/random/randomizer.c

Lines 97 to 99 in 7c41dd9

result = randomizer->algo->generate(randomizer->status);

if (randomizer->status->last_generated_size > sizeof(zend_long)) {

zend_throw_exception(spl_ce_RuntimeException, "Generated value exceeds size of int", 0);

and

php-src/ext/random/engine_user.c

Lines 36 to 40 in 7c41dd9

/* Guard for over 64-bit results */

if (size > sizeof(uint64_t)) {

size = sizeof(uint64_t);

}

status->last_generated_size = size;

and that sizeof(zend_long) is equal to sizeof(uint64_t) on 64-bit systems), but as I said, there were 2 occurrences in that file (compatibility_user.phpt), but you only removed the first one; did you "forget" to also remove the second one (

php-src/ext/random/tests/03_randomizer/compatibility_user.phpt

Lines 72 to 74 in 7c41dd9

if (\PHP_INT_SIZE >= 8) {

throw $e;

}

) or is it somewhat useful for Xoshiro256StarStar (although it apparently wasn't for PcgOneseq128XslRr64)?

Thank you for your calm and measured point of view. I was totally confused. From what I see, the guard test is unnecessary as it is tested in get_bytes.phpt. I have removed the if completely.

fixed: 5144026

cmb69

Thank you! I'm very much in favor of splitting the method in two. The patch looks good to me.

TimWolla

The diff looks reasonable to me. Just one remark to clarify that nextInt() will return positive numbers only, as the most-significant bit is shifted out.

I guess the release managers should decide on this one?

ext/random/randomizer.c

cmb69 · 2022-07-22T16:08:20Z

I guess the release managers should decide on this one?

@ramsey, @saundefined, @adoy, any objections?

If this gets merged, the RFC should probably be amended, so that the doc team won't put outdated information in the migration guide.

ramsey · 2022-07-22T18:02:57Z

Will this change the RFC in any way that would have caused any voters to vote "no" had this been part of the original RFC?

zeriyoshi · 2022-07-23T15:11:26Z

@ramsey

Will this change the RFC in any way that would have caused any voters to vote "no" had this been part of the original RFC?

Since the method is only separated, I don't think anyone would be negative about it, or that it would be so negative that it would be rejected if a vote were actually taken.

Nevertheless, this is a very exceptional measure and may set a bad precedent. As PHP 8.2 Release Manager, can you hearing the ML if they have any objections to this change?

I apologize for the extra hassle this has caused.

guilliamxavier · 2022-07-25T08:20:24Z

Specific ML thread (created shortly after this PR): https://externals.io/message/118290
(only positive feedback so far, in that thread and the previous one)

Nevertheless, this is a very exceptional measure and may set a bad precedent

There has already been precedents, e.g. https://externals.io/message/117136, not necessarily "bad" ;)

TimWolla · 2022-07-30T09:26:10Z

It might be useful to get a decision here, so that this can go into Beta 2 (or not).

Since the method is only separated, I don't think anyone would be negative about it, or that it would be so negative that it would be rejected if a vote were actually taken.

I agree with zeriyoshi's argument here. The same functionality is still available, just separated into two independent methods with a cleaner method signature for each of them.

cmb69 · 2022-07-30T10:33:57Z

If there are no objections, I'll merge this on Monday.

@zeriyoshi, could you please fix the merge conflicts?

…xtInt() Since argument overloading is not safe for reflection, the method needed to be split appropriately.

Co-authored-by: Tim Düsterhus <[email protected]>

github-actions bot added the Extension: random label Jul 20, 2022

zeriyoshi mentioned this pull request Jul 20, 2022

Random Extension 5.x follow-up #9052

Closed

kocsismate approved these changes Jul 20, 2022

View reviewed changes

guilliamxavier reviewed Jul 20, 2022

View reviewed changes

zeriyoshi force-pushed the upstream/split_randomizer_method branch from 53e82e4 to d78c647 Compare July 20, 2022 13:47

TimWolla self-requested a review July 20, 2022 15:58

zeriyoshi force-pushed the upstream/split_randomizer_method branch from 7c41dd9 to 5144026 Compare July 21, 2022 14:21

cmb69 approved these changes Jul 22, 2022

View reviewed changes

TimWolla approved these changes Jul 22, 2022

View reviewed changes

ext/random/randomizer.c Outdated Show resolved Hide resolved

zeriyoshi force-pushed the upstream/split_randomizer_method branch from 8885bad to a92af01 Compare July 23, 2022 15:23

zeriyoshi and others added 6 commits July 30, 2022 21:00

random: split Randomizer::getInt() without argument to Randomizer::ne…

76e1e1b

…xtInt() Since argument overloading is not safe for reflection, the method needed to be split appropriately.

ci: kick

da679e1

random: test: fix test

c645fc9

random: test: remove useless if

af43348

random: test: remove unnecessary if

ce29b62

Update ext/random/randomizer.c

908cb0b

Co-authored-by: Tim Düsterhus <[email protected]>

zeriyoshi force-pushed the upstream/split_randomizer_method branch from a92af01 to 908cb0b Compare July 30, 2022 12:00

update NEWS

ad6fd76

cmb69 closed this in 4e92c74 Aug 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[random] split getInt() with no arguments into nextInt() #9057

[random] split getInt() with no arguments into nextInt() #9057

zeriyoshi commented Jul 20, 2022

guilliamxavier Jul 20, 2022 •

edited

Loading

zeriyoshi Jul 20, 2022

zeriyoshi Jul 20, 2022

zeriyoshi Jul 20, 2022

guilliamxavier Jul 20, 2022 •

edited

Loading

zeriyoshi Jul 20, 2022

guilliamxavier Jul 20, 2022

zeriyoshi Jul 21, 2022

guilliamxavier Jul 21, 2022

zeriyoshi Jul 21, 2022

cmb69 left a comment

TimWolla left a comment •

edited

Loading

cmb69 commented Jul 22, 2022

ramsey commented Jul 22, 2022

zeriyoshi commented Jul 23, 2022

guilliamxavier commented Jul 25, 2022 •

edited

Loading

TimWolla commented Jul 30, 2022

cmb69 commented Jul 30, 2022

	} catch (\RuntimeException $e) {
	if (\PHP_INT_SIZE >= 8) {
	throw $e;
	}
	if ($e->getMessage() !== 'Generated value exceeds size of int') {
	throw $e;
	}
	}

	result = randomizer->algo->generate(randomizer->status);
	if (randomizer->status->last_generated_size > sizeof(zend_long)) {
	zend_throw_exception(spl_ce_RuntimeException, "Generated value exceeds size of int", 0);

	/* Guard for over 64-bit results */
	if (size > sizeof(uint64_t)) {
	size = sizeof(uint64_t);
	}
	status->last_generated_size = size;

[random] split getInt() with no arguments into nextInt() #9057

[random] split getInt() with no arguments into nextInt() #9057

Conversation

zeriyoshi commented Jul 20, 2022

guilliamxavier Jul 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guilliamxavier Jul 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmb69 left a comment

Choose a reason for hiding this comment

TimWolla left a comment • edited Loading

Choose a reason for hiding this comment

cmb69 commented Jul 22, 2022

ramsey commented Jul 22, 2022

zeriyoshi commented Jul 23, 2022

guilliamxavier commented Jul 25, 2022 • edited Loading

TimWolla commented Jul 30, 2022

cmb69 commented Jul 30, 2022

guilliamxavier Jul 20, 2022 •

edited

Loading

guilliamxavier Jul 20, 2022 •

edited

Loading

TimWolla left a comment •

edited

Loading

guilliamxavier commented Jul 25, 2022 •

edited

Loading