# [VOTE] Random Extension Improvement - Thread: https://externals.io/message/118163 - Original archive: https://news-web.php.net/php.internals/118163 - Started by Go Kudo on 2022-07-03T03:00:00+00:00 - Messages: 16 - RFC: Random Extension Improvement (Implemented (PHP 8.2)) — https://externals.io/rfc/random_extension_improvement.md - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · RFCs at https://externals.io/rfc.md · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #118163 — Go Kudo — 2022-07-03T03:00:00+00:00 URL: https://externals.io/message/118163#118163 Hello internals. Voting began on 2022-07-02 03:00:00 (UTC) and will end on 2022-07-16 03:00:00 (UTC). https://wiki.php.net/rfc/random_extension_improvement Best Regards, Go Kudo --- ## #118203 — Go Kudo — 2022-07-06T05:06:51+00:00 URL: https://externals.io/message/118163#118203 · In reply to: #118163 2022年7月3日(日) 12:00 Go Kudo : > Hello internals. > > Voting began on 2022-07-02 03:00:00 (UTC) and will end on 2022-07-16 > 03:00:00 (UTC). > > https://wiki.php.net/rfc/random_extension_improvement > > Best Regards, > Go Kudo > Hi. Once again, I apologize for the delay in starting the voting due to various reasons. One point, the RFC Status was still Under Discussion, which has been corrected. No other changes have been made to the text. I will be able to resume work today and will continue implementation and refactoring with PHP 8.2 in mind. P.S. > Add Randomizer::pickArrayKeys(array $array, int $num): array method This seems to be relatively unpopular, and I would like to know the reasons for those who voted No. Is it because this function, like array_rand, is overly complex in its behavior? Regards, Go Kudo --- ## #118217 — Go Kudo — 2022-07-07T15:52:34+00:00 URL: https://externals.io/message/118163#118217 · In reply to: #118163 2022年7月3日(日) 12:00 Go Kudo : > Hello internals. > > Voting began on 2022-07-02 03:00:00 (UTC) and will end on 2022-07-16 > 03:00:00 (UTC). > > https://wiki.php.net/rfc/random_extension_improvement > > Best Regards, > Go Kudo > Hi. Implementation is now proceeding. It includes fixes to some of the issues that were pointed out previously. https://github.com/php/php-src/pull/8094 `Randomizer::arrayPickKeys()` is currently not implemented for now, since it is most likely to be rejected. (Of course, we are ready to revert to the other items if they are also rejected.) I want to see if it looks good to you. Regards, Go Kudo --- ## #118218 — tim@bastelstu.be — 2022-07-07T16:43:56+00:00 URL: https://externals.io/message/118163#118218 · In reply to: #118217 Hi > Implementation is now proceeding. > It includes fixes to some of the issues that were pointed out previously. > > https://github.com/php/php-src/pull/8094 > > `Randomizer::arrayPickKeys()` is currently not implemented for now, since > it is most likely to be rejected. > (Of course, we are ready to revert to the other items if they are also > rejected.) > > I want to see if it looks good to you. I've reviewed the current implementation of the engines with a specific focus on the tests. 1. I have verified the Xoshiro256** tests against a pure PHP implementation (which I verified against the C reference implementation [1]). 2. I have verified the Pcg64OneseqXslRr64 pcgoneseq128xslrr64_value.phpt test against the reference C implementation [2] with the attached pcg-verification.c. 3. I have verified the Mt19937 mt_value.phpt test against the C++ standard library implementation [3] with the attached mt-verification.cc. The three engines do what they are supposed to do. I'm not qualified to review with regard to PHP Internals best practices. It would be good if someone with the necessary expertise could proceed with that part of the review. Best regards Tim Düsterhus [1] https://prng.di.unimi.it/xoshiro256starstar.c [2] https://www.pcg-random.org/download.html#c-implementation [3] https://en.cppreference.com/w/cpp/numeric/random/mersenne_twister_engine --- ## #118251 — Go Kudo — 2022-07-12T12:21:29+00:00 URL: https://externals.io/message/118163#118251 · In reply to: #118218 2022年7月8日(金) 1:44 Tim Düsterhus : > Hi > > > > […] > > I've reviewed the current implementation of the engines with a specific > focus on the tests. > > 1. I have verified the Xoshiro256** tests against a pure PHP > implementation (which I verified against the C reference implementation > [1]). > > 2. I have verified the Pcg64OneseqXslRr64 pcgoneseq128xslrr64_value.phpt > test against the reference C implementation [2] with the attached > pcg-verification.c. > > 3. I have verified the Mt19937 mt_value.phpt test against the C++ > standard library implementation [3] with the attached mt-verification.cc. > > The three engines do what they are supposed to do. I'm not qualified to > review with regard to PHP Internals best practices. It would be good if > someone with the necessary expertise could proceed with that part of the > review. > > Best regards > Tim Düsterhus > > [1] https://prng.di.unimi.it/xoshiro256starstar.c > [2] https://www.pcg-random.org/download.html#c-implementation > [3] > https://en.cppreference.com/w/cpp/numeric/random/mersenne_twister_engine Hi Tim. Sorry for the late reply. Thanks for the verification. I changed the serialization format to hex string as you pointed out earlier. You should now be able to validate your PHP implementation. Regards, Go Kudo --- ## #118252 — Go Kudo — 2022-07-12T12:26:53+00:00 URL: https://externals.io/message/118163#118252 · In reply to: #118163 2022年7月3日(日) 12:00 Go Kudo : > Hello internals. > > Voting began on 2022-07-02 03:00:00 (UTC) and will end on 2022-07-16 > 03:00:00 (UTC). > > https://wiki.php.net/rfc/random_extension_improvement > > Best Regards, > Go Kudo > Hi Internals. Currently, the renaming of Randomizer::shuffleString() to Randomizer::shuffleBytes() seems acceptable. I forgot to note the change regarding arguments when I submitted this RFC. With this change, the argument was supposed to be changed from `string $string` to `string $bytes`. - https://github.com/php/php-src/pull/8094#discussion_r916630626 Is this change acceptable? Or should I keep `string $string`? Regards, Go Kudo --- ## #118254 — Guilliam Xavier — 2022-07-12T13:42:13+00:00 URL: https://externals.io/message/118163#118254 · In reply to: #118252 > > […] > > Hi Internals. > > Currently, the renaming of Randomizer::shuffleString() to > Randomizer::shuffleBytes() seems acceptable. > > I forgot to note the change regarding arguments when I submitted this RFC. > With this change, the argument was supposed to be changed from `string > $string` to `string $bytes`. > > - https://github.com/php/php-src/pull/8094#discussion_r916630626 For reference, first was https://github.com/php/php-src/pull/8094/commits/32e83a71eae67fdd822fd12ded5a8c054024a4da#r916058794 > Is this change acceptable? Or should I keep `string $string`? --- ## #118257 — tim@bastelstu.be — 2022-07-12T16:04:21+00:00 URL: https://externals.io/message/118163#118257 · In reply to: #118252 Hi > Currently, the renaming of Randomizer::shuffleString() to > Randomizer::shuffleBytes() seems acceptable. > > I forgot to note the change regarding arguments when I submitted this RFC. > With this change, the argument was supposed to be changed from `string > $string` to `string $bytes`. > > - https://github.com/php/php-src/pull/8094#discussion_r916630626 > > Is this change acceptable? Or should I keep `string $string`? I believe if the method name changes, then the parameter names are also open for adjustment. Especially in this case of a single parameter method where it is unlikely that named parameters are used by the developer. I also think that both '$string' and '$binary' are appropriate parameter names in this case, so particular preference from my side. Best regards Tim Düsterhus --- ## #118258 — tim@bastelstu.be — 2022-07-12T16:10:43+00:00 URL: https://externals.io/message/118163#118258 · In reply to: #118257 Hi > I also think that both '$string' and '$binary' are appropriate parameter > names in this case, so particular preference from my side. Sorry for the follow-up, there's two mistakes in that sentence. It should read: I also think that both '$string' and **'$bytes'** are appropriate parameter names in this case, so **no** particular preference from my side. Best regards Tim Düsterhus --- ## #118262 — Go Kudo — 2022-07-14T02:08:15+00:00 URL: https://externals.io/message/118163#118262 · In reply to: #118258 2022年7月13日(水) 1:10 Tim Düsterhus : > Hi > > > > […] > > Sorry for the follow-up, there's two mistakes in that sentence. It > should read: > > I also think that both '$string' and **'$bytes'** are appropriate > parameter names in this case, so **no** particular preference from my side. > > Best regards > Tim Düsterhus > Hi I agree with you. I will change the parameter name from `$string` to `$bytes` as I don't see any problem. I will try to explain the changes more rigorously in future proposals. Thank you. Regards, Go Kudo --- ## #118263 — Guilliam Xavier — 2022-07-14T04:32:02+00:00 URL: https://externals.io/message/118163#118263 · In reply to: #118262 > 2022年7月13日(水) 1:10 Tim Düsterhus : > > > […] > > Hi > > I agree with you. I will change the parameter name from `$string` to > `$bytes` as I don't see any problem. > > I will try to explain the changes more rigorously in future proposals. > Thank you. > > Regards, > Go Kudo > Hi, I was waiting for more opinions but... so here's mine: I would prefer to keep "$string", as [that's how I read the RFCs, and] when calling e.g. shuffleBytes('foobar') I don't feel like I'm passing "bytes" (or "a binary") but a string (to be shuffled byte-wise rather than character-wise or codepoint-wise, but that's from the function, not the argument)... Granted, not compelling, and probably won't matter in practice, but hey ;) Regards PS: sent from mobile -- Guilliam Xavier --- ## #118264 — Claude Pache — 2022-07-14T07:13:36+00:00 URL: https://externals.io/message/118163#118264 · In reply to: #118263 > Le 14 juil. 2022 à 06:32, Guilliam Xavier a écrit : > > > > > […] > > Hi, > > I was waiting for more opinions but... so here's mine: > > I would prefer to keep "$string", as [that's how I read the RFCs, and] when > calling e.g. shuffleBytes('foobar') I don't feel like I'm passing "bytes" > (or "a binary") but a string (to be shuffled byte-wise rather than > character-wise or codepoint-wise, but that's from the function, not the > argument)... > Granted, not compelling, and probably won't matter in practice, but hey ;) > > Regards > > PS: sent from mobile > > > -- > Guilliam Xavier I agree with Guilliam: this function is about «shuffling the bytes of the given string» (as opposed to, say, «array of ints») As precedent, there are `bin2hex($string)` and `md5($string)`, which are unambiguously working with bytes from data given in the form of string, where «string» is a PHP type, which can hold data that is not necessarily UTF-8-encoded or Shift-JIS-encoded text. —Claude --- ## #118266 — Go Kudo — 2022-07-15T16:02:02+00:00 URL: https://externals.io/message/118163#118266 · In reply to: #118264 2022年7月14日(木) 16:14 Claude Pache : > > > > […] > écrit : > > […] > parameter > > […] > when > > […] > ;) > > […] > > I agree with Guilliam: this function is about «shuffling the bytes of the > given string» (as opposed to, say, «array of ints») > > As precedent, there are `bin2hex($string)` and `md5($string)`, which are > unambiguously working with bytes from data given in the form of string, > where «string» is a PHP type, which can hold data that is not necessarily > UTF-8-encoded or Shift-JIS-encoded text. > > —Claude > > -- > > To unsubscribe, visit: https://www.php.net/unsub.php > > Thanks for the input. This is certainly something to consider. For example, what about the `$binaryString` argument name? I think it is clearer. I have adopted this as the name of the internal API: `php_binary_string_shuffle()`. As stated in the GitHub PR, this implementation probably requires additional changes. After voting on the current RFC is complete, we will again send an email to the ML. Regards, Go Kudo --- ## #118269 — Go Kudo — 2022-07-16T06:11:01+00:00 URL: https://externals.io/message/118163#118269 · In reply to: #118163 2022年7月3日(日) 12:00 Go Kudo : > Hello internals. > > Voting began on 2022-07-02 03:00:00 (UTC) and will end on 2022-07-16 > 03:00:00 (UTC). > > https://wiki.php.net/rfc/random_extension_improvement > > Best Regards, > Go Kudo > Hi Voting was closed and all proposals were accepted. https://wiki.php.net/rfc/random_extension_improvement https://github.com/php/php-src/pull/8094 However, as pointed out in the PR, there are still areas that need to be improved and corrected. First of all, overloading of `getInt()` should be stopped. This is due to the fact that the signature of the `mt_rand()` function is still used, but it was pointed out that no new implementation should be made. https://github.com/php/php-src/pull/8094/files#r919693108 Second, there is an issue regarding the handling of header files for compatibility. This was left in because it was considered necessary in previous discussions, but it has been suggested that it is unnecessary. I too would like to remove it if possible. https://github.com/php/php-src/pull/8094/files#r917308676 However, if you try to make this change by creating an RFC as usual, you are already out of time. I am really confused and don't really know what to do about this. Can anyone give me some good ideas? Best Regards, Go Kudo --- ## #118272 — Guilliam Xavier — 2022-07-18T11:18:56+00:00 URL: https://externals.io/message/118163#118272 · In reply to: #118266 > For example, what about the `$binaryString` argument name? I would recommend to either keep `$string` (conservative) or rename it to `$bytes` (consistent with the new function name) [I interpret the RFCs as the former, but the implementation is currently as the latter]. About your other mail, I too like the idea to split `getInt(int $min = UNKNOWN, int $max = UNKNOWN)` into distinct `nextInt()` and `getInt(int $min, int $max)` [in a subsequent PR, IIUC]. Regards, -- Guilliam Xavier --- ## #118288 — tim@bastelstu.be — 2022-07-19T20:22:04+00:00 URL: https://externals.io/message/118163#118288 · In reply to: #118269 Hi > However, as pointed out in the PR, there are still areas that need to be > improved and corrected. > > First of all, overloading of `getInt()` should be stopped. This is due to > the fact that the signature of the `mt_rand()` function is still used, but > it was pointed out that no new implementation should be made. > > https://github.com/php/php-src/pull/8094/files#r919693108 > > Second, there is an issue regarding the handling of header files for > compatibility. This was left in because it was considered necessary in > previous discussions, but it has been suggested that it is unnecessary. I > too would like to remove it if possible. > > https://github.com/php/php-src/pull/8094/files#r917308676 > > However, if you try to make this change by creating an RFC as usual, you > are already out of time. > > I am really confused and don't really know what to do about this. > > Can anyone give me some good ideas? For those following along, the follow-up PR is here: https://github.com/php/php-src/pull/9052 Best regards Tim Düsterhus