Skip to content

Random Extension 5.x follow-up #9052

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

Closed

Conversation

zeriyoshi
Copy link
Contributor

@zeriyoshi zeriyoshi commented Jul 19, 2022

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.

This changes separates the argumentless Random\Randomizer::getInt() into Random\Randomizer::nextInt().

At the same time, compatibility header files for internal APIs were removed.
This may affect downstream extensions, but can be easily circumvented by macro-branching with PHP_VERSION_ID.

@TimWolla
Copy link
Member

Thanks. I've already picked the first comment for the change to EXTENSIONS into d43e55b, because that one definitely is good, whereas the rest will need some discussion.

// nextInt
for ($i = 0; $i < 1000; $i++) {
try {
if (!\is_int($randomizer->nextInt())) {
Copy link
Member

Choose a reason for hiding this comment

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

The result cannot be not int due to the return type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in new PR: #9057

@TimWolla
Copy link
Member

@zeriyoshi One request for the future: Can you split unrelated changes into separate PRs? In this case: One for the EXTENSIONS file (which I already merged), one for the headers and one for getInt/nextInt. That makes it easier to independently review and merge them.

@zeriyoshi
Copy link
Contributor Author

@TimWolla
OK, This PR will focus on nextInt() / getInt().

@zeriyoshi zeriyoshi changed the title Random Extension 5.x follow-up [Random] split getInt() with no arguments into nextInt() Jul 20, 2022
@zeriyoshi zeriyoshi changed the title [Random] split getInt() with no arguments into nextInt() Random Extension 5.x follow-up Jul 20, 2022
@zeriyoshi
Copy link
Contributor Author

Since it is complicated, I would like to close this PR and create a new PR.

@zeriyoshi zeriyoshi closed this Jul 20, 2022
@zeriyoshi
Copy link
Contributor Author

zeriyoshi commented Jul 20, 2022

@TimWolla @kocsismate
These PRs were separated. (It appears that the EXTENSIONS changes are already in master. Thanks)

@iluuu1994
Copy link
Member

This caused two UBSAN failures:

https://github.com/php/php-src/runs/7420828169?check_suite_focus=true

========DIFF========
001+ /home/runner/work/php-src/php-src/ext/random/random.c:322:24: runtime error: signed integer overflow: 9223372036854775807 - -9223372036854775808 cannot be represented in type 'long int'
     Random\Engine\Mt19937: success
     Random\Engine\Mt19937: success
005+ /home/runner/work/php-src/php-src/ext/random/random.c:325:55: runtime error: signed integer overflow: -9223372036854775808 + -1050901053848098914 cannot be represented in type 'long int'
     Random\Engine\Xoshiro256StarStar: success
     Serialization of 'Random\Engine\Secure' is not allowed
     Serialization of 'Random\Engine@anonymous' is not allowed
--
========DONE========
FAIL Random: Randomizer: serialize [ext/random/tests/03_randomizer/serialize.phpt] 

========DIFF========
001+ /home/runner/work/php-src/php-src/ext/random/random.c:322:24: runtime error: signed integer overflow: 9223372036854775807 - -9223372036854775808 cannot be represented in type 'long int'
     Random number generation failed
     Random number generation failed
     Random number generation failed
006+ /home/runner/work/php-src/php-src/ext/random/random.c:325:55: runtime error: signed integer overflow: -9223372036854775808 + -1 cannot be represented in type 'long int'
     int(%d)
     string(2) "ff"
     Random number generation failed
--
========DONE========
FAIL Random: Randomizer: User: Engine unsafe [ext/random/tests/03_randomizer/user_unsafe.phpt] 

@zeriyoshi Could you have a look? Thanks!

@zeriyoshi
Copy link
Contributor Author

zeriyoshi commented Jul 20, 2022

This caused two UBSAN failures:

https://github.com/php/php-src/runs/7420828169?check_suite_focus=true

========DIFF========
001+ /home/runner/work/php-src/php-src/ext/random/random.c:322:24: runtime error: signed integer overflow: 9223372036854775807 - -9223372036854775808 cannot be represented in type 'long int'
     Random\Engine\Mt19937: success
     Random\Engine\Mt19937: success
005+ /home/runner/work/php-src/php-src/ext/random/random.c:325:55: runtime error: signed integer overflow: -9223372036854775808 + -1050901053848098914 cannot be represented in type 'long int'
     Random\Engine\Xoshiro256StarStar: success
     Serialization of 'Random\Engine\Secure' is not allowed
     Serialization of 'Random\Engine@anonymous' is not allowed
--
========DONE========
FAIL Random: Randomizer: serialize [ext/random/tests/03_randomizer/serialize.phpt] 

========DIFF========
001+ /home/runner/work/php-src/php-src/ext/random/random.c:322:24: runtime error: signed integer overflow: 9223372036854775807 - -9223372036854775808 cannot be represented in type 'long int'
     Random number generation failed
     Random number generation failed
     Random number generation failed
006+ /home/runner/work/php-src/php-src/ext/random/random.c:325:55: runtime error: signed integer overflow: -9223372036854775808 + -1 cannot be represented in type 'long int'
     int(%d)
     string(2) "ff"
     Random number generation failed
--
========DONE========
FAIL Random: Randomizer: User: Engine unsafe [ext/random/tests/03_randomizer/user_unsafe.phpt] 

@zeriyoshi Could you have a look? Thanks!

@iluuu1994
Thanks!
I fixed it. Could you please check? #9060

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.

4 participants