On Mon, Jan 18, 2021 at 5:29 PM Go Kudo <zeriyo...@gmail.com> wrote: > RFC and implementation updated. > > https://wiki.php.net/rfc/object_scope_prng > https://github.com/php/php-src/pull/6568 > > > MT19937 returns a signed 32bit number as a result of the `next()` method; > the mt_rand() function returns a bit-shifted unsigned 31bit number, so the > results are not identical. > > The `rng_range()` function has been renamed to `rng_rand()` and returns an > unsigned 31-bit integer like the `mt_rand()` function if only the $rng > argument is received. > > ```php > mt_srand(1234); > mt_rand() === rng_rand(new \RNG\MT19937(1234)); // true > ``` > > This allows to completely replace the `mt_rand()` function with the > RNG\MT19937. > > I said I wanted to initiate a vote, but there seems to be too much > discussion missing for that. > I'd like to wait and see for a while. > > Regards, > Go Kudo > > 2021年1月17日(日) 20:02 Go Kudo <zeriyo...@gmail.com>: > > > Updated the RFC and fixed the implementation. > > Also made some additions to the RFC about when this feature might be > > useful. > > > > RFC: https://wiki.php.net/rfc/object_scope_prng > > Implementation PR: https://github.com/php/php-src/pull/6568 (All CI > > passed) > > > > The main points are as follows: > > > > - The implementation now meets all requirements. (maybe) > > - Implemented MT19937, which is compatible with PHP standard MT. The > > consistency of these is ensured by tests. > > - The implementation is now what we used to call TypeII. > > - All RNG implementations can now be inherited like regular classes. Use > > faster calls only if the class is an internal class. > > - The implementation has been heavily modified and the quality of the > code > > has been improved. > > > > Currently, there are a few concerns: > > > > - MT19937 returns a signed 32bit number as a result of the `next()` > > method; the mt_rand() function returns a bit-shifted unsigned 31bit > number, > > so the results are not identical. > > - You can get the equivalent result with `$rng->next() << 1 & > > PHP_INT_MAX`, but maybe you should have a `nextUnsigned31()` method. > > - Currently, the test uses `$rng->next() << 1 & PHP_INT_MAX`. > > - The implementation of MT19937 is redundant. This can be merged. > > > > Anyway, I think the code is finally ready for review. > > Once that is done, we will start voting on RFCs. > > > > Regards, > > Go Kudo >
Overall I like the proposal. Some notes: * I know you only just changed it, but I don't think rng_rand() should support calling without arguments. This is a backwards-compatibility leftover in mt_rand() and we should not carry it over into a new API. * You should probably mention that the classes can be serialized in the RFC. Maybe you want to write out all the methods so it's also visible how the constructors for the classes look like. * Naming: RNG\RNGInterface seems a bit redundant. Of course, we can't make it RNG\Interface. RNG\Generator would be an option. I'm not sure about this. Maybe RNG\RNGInterface is fine. * 64-bit: I looked over your implementation, and I think your approach to handling 64-bits is correct. You only call next64() if the requested range is larger than 32-bit, and that can only happen on 64-bit systems, thus guaranteeing consistency of results (where they can be consistent at all). What I'm unsure about is the way this gets exposed to userland via next() and next64() methods. I think fetching of 64-bit values is mainly interesting as an internal optimization, and not so much important to be part of the RNGInterface API. The user is intended to get numbers via rng_rand(), not by directly calling next(). A possibility here would be to drop the RNG64Interface and next64() method, have next() always return 32-bit, but retain the internal next64 API. For this to make sense, we'd need two subsequent internal next() calls to return the same data as a single next64() call (i.e. store the result and first return one half then the other). Would this make sense to you? Regards, Nikita
