Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » [VOTE] Random Extension Improvement »

[VOTE] Random Extension Improvement

php.internals

Go Kudo

4 years ago
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

Go Kudo

4 years ago
2022年7月3日(日) 12:00 Go Kudo <g-kudo@colopl.co.jp>:
> 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

zeriyoshi

4 years ago
2022年7月3日(日) 12:00 Go Kudo <g-kudo@colopl.co.jp>:
> 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

Tim Düsterhus

4 years ago
Hi On 7/7/22 17:52, Go Kudo wrote:
> 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

Go Kudo

4 years ago
2022年7月8日(金) 1:44 Tim Düsterhus <tim@bastelstu.be>:
> Hi > > On 7/7/22 17:52, Go Kudo wrote: > > 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
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

Go Kudo

4 years ago
2022年7月3日(日) 12:00 Go Kudo <g-kudo@colopl.co.jp>:
> 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

Guilliam Xavier

4 years ago
> > https://wiki.php.net/rfc/random_extension_improvement > > 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

Tim Düsterhus

4 years ago
Hi On 7/12/22 14:26, Go Kudo wrote:
> 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

Tim Düsterhus

4 years ago
Hi On 7/12/22 18:04, Tim Düsterhus wrote:
> 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

Go Kudo

4 years ago
2022年7月13日(水) 1:10 Tim Düsterhus <tim@bastelstu.be>:
> Hi > > On 7/12/22 18:04, Tim Düsterhus wrote: > > 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 >
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

Guilliam Xavier

4 years ago
On Thursday, July 14, 2022, Go Kudo <g-kudo@colopl.co.jp> wrote:
> 2022年7月13日(水) 1:10 Tim Düsterhus <tim@bastelstu.be>: > >> Hi >> >> On 7/12/22 18:04, Tim Düsterhus wrote: >> > 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 >> > > 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

Claude Pache

4 years ago
> Le 14 juil. 2022 à 06:32, Guilliam Xavier <guilliam.xavier@gmail.com> a écrit : > > On Thursday, July 14, 2022, Go Kudo <g-kudo@colopl.co.jp> wrote: > >> 2022年7月13日(水) 1:10 Tim Düsterhus <tim@bastelstu.be>: >> >>> Hi >>> >>> On 7/12/22 18:04, Tim Düsterhus wrote: >>>> 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 >>> >> >> 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
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

zeriyoshi

4 years ago
2022年7月14日(木) 16:14 Claude Pache <claude.pache@gmail.com>:
> > > > Le 14 juil. 2022 à 06:32, Guilliam Xavier <guilliam.xavier@gmail.com> a > écrit : > > > > On Thursday, July 14, 2022, Go Kudo <g-kudo@colopl.co.jp> wrote: > > > >> 2022年7月13日(水) 1:10 Tim Düsterhus <tim@bastelstu.be>: > >> > >>> Hi > >>> > >>> On 7/12/22 18:04, Tim Düsterhus wrote: > >>>> 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 > >>> > >> > >> 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 > > 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 > > -- > PHP Internals - PHP Runtime Development Mailing List > 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

Guilliam Xavier

4 years ago
> 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

zeriyoshi

4 years ago
2022年7月3日(日) 12:00 Go Kudo <g-kudo@colopl.co.jp>:
> 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

Tim Düsterhus

4 years ago
Hi On 7/16/22 08:11, Go Kudo wrote:
> 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