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
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
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
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.
-
I have verified the Xoshiro256** tests against a pure PHP
implementation (which I verified against the C reference implementation
[1]). -
I have verified the Pcg64OneseqXslRr64 pcgoneseq128xslrr64_value.phpt
test against the reference C implementation [2] with the attached
pcg-verification.c. -
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
2022年7月8日(金) 1:44 Tim Düsterhus tim@bastelstu.be:
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.
I have verified the Xoshiro256** tests against a pure PHP
implementation (which I verified against the C reference implementation
[1]).I have verified the Pcg64OneseqXslRr64 pcgoneseq128xslrr64_value.phpt
test against the reference C implementation [2] with the attached
pcg-verification.c.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
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.
Is this change acceptable? Or should I keep string $string?
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 fromstring $stringtostring $bytes.
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?
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 fromstring $stringtostring $bytes.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
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
2022年7月13日(水) 1:10 Tim Düsterhus tim@bastelstu.be:
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
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
2022年7月13日(水) 1:10 Tim Düsterhus tim@bastelstu.be:
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üsterhusHi
I agree with you. I will change the parameter name from
$stringto
$bytesas 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
Le 14 juil. 2022 à 06:32, Guilliam Xavier guilliam.xavier@gmail.com a écrit :
2022年7月13日(水) 1:10 Tim Düsterhus tim@bastelstu.be:
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üsterhusHi
I agree with you. I will change the parameter name from
$stringto
$bytesas I don't see any problem.I will try to explain the changes more rigorously in future proposals.
Thank you.Regards,
Go KudoHi,
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
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 :2022年7月13日(水) 1:10 Tim Düsterhus tim@bastelstu.be:
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üsterhusHi
I agree with you. I will change the parameter name from
$stringto
$bytesas I don't see any problem.I will try to explain the changes more rigorously in future proposals.
Thank you.Regards,
Go KudoHi,
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 XavierI 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)andmd5($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
For example, what about the
$binaryStringargument 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
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
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 themt_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