Hello Go Kudo, On Tue, Jun 8, 2021 at 2:33 PM Go Kudo <zeriyo...@gmail.com> wrote:
> Hello iinternals. > > There doesn't seem to be much mention of it. > But, that may be because it has been discussed well in advance. > Thank you for participating in the discussion. > I'm not sure you really addressed https://externals.io/message/114561#114680 ? especially this part: """ If you pick the second option, then you should consistently separate the source for both extension-provided RNGs and user-provided ones. I don't think it makes sense to have extension provided RNGs use a `new Random(RANDOM_XORSHIFT128PLUS)` interface, while user-provided ones use `new Random(new SomeRandomSource)`. Going down this route, it should be `new Random(new XorShift128Plus)`. This is a pure design, which also means that it's more complicated, and may make simple usages harder (though there is certainly room for a Random::createDefault() here.) """ Now, that might sound like it would take us back to the "full OO" version (with multiple classes), which was deemed too "user-*un*friendly" by several people (including me :s), *but* there are some new ideas to consider, e.g. (for the Random class): - add a `public static function createDefault(): self` as suggested by Nikita, or - change `public function __construct` parameters to e.g. `(string $algo = XorShift128Plus::class, ?int $seed = null, mixed ...$extra_args)` (that would internally do a `new $algo($seed, ...$extra_args)` to store), to be called not as `new Random(new UserDefinedRNG($seed_or_null, $foo, $bar))` but as `new Random(UserDefinedRNG::class, $seed_or_null, $foo, $bar)`. Any new opinions (or other ideas)? > Now, if there is no particular discussion on this, I will try to start the > voting phase next week. > Of course, I will contact you separately. > > However, I was looking back at the implementation and found only one point > of concern. > > With the current implementation, the results of the following example will > match. > > ```php > $one = new Random(); > $one->nextInt(); > $two = clone $one; > > var_dump($one->nextInt() === $two->nextInt()); // true > ``` > > But, this is not true for user implementations. > > ```php > class UserRNG implements RandomNumberGenerator > { > protected int $current = 0; > > public function generate(): int > { > return ++$this->current; > } > } > > $rng = new UserRNG(); > $one = new Random($rng); > $one->nextInt(); > $two = clone $one; > > var_dump($one->nextInt() === $two->nextInt()); // false > ``` > > This is because `$rng` is kept as a normal property and is managed by the > standard PHP mechanism. In other words, it is passed by reference. > > This is not consistent with the built-in RNG behavior. However, I don't see > a problem with this behavior. > I feel that the standard PHP behavior is preferable to changing the > userland behavior in a specific way. > > I would like to solicit opinions. > Couldn't the Random class simply implement `public function __clone(): void` (that would internally do like `$this->algo = clone $this->algo;`)? > > Regards, > Go Kudo > > 2021年6月1日(火) 23:28 Go Kudo <zeriyo...@gmail.com>: > > > Hello internals. > > Thanks for continuing to participate in the discussion. > > > > I've sorted out the proposal, and finished writing and implementing the > > RFC. > > (Funny, I know.) I think it's time to start a discussion. > > > > https://wiki.php.net/rfc/rng_extension > > https://github.com/php/php-src/pull/7079 > > > > The main changes since last time are as follows: > > > > - The ugly RANDOM_USER has been removed. > > - RandomNumberGenerator interface has been added for user-defined RNGs. > > - Random class is now final. > > - Random class now accepts a RandomNumberGenerator interface other than > > string as the first argument to the constructor. > > - INI directive has been removed. In 32-bit environments, the result is > > always truncated. > > > > What I'm struggling with now is the behavior when calling nextInt() in a > > 32-bit environment using a 64-bit RNG. It currently returns a truncated > > result, which means that the code loses compatibility with the result of > > running on a 64-bit machine. > > I was also considering throwing an exception, but which would be > > preferable? > Indeed, a decision has to be made. If you throw an exception, we wouldn't have the "issue" of different results on 32- vs 64-bit machines, but XorShift128+ and Secure would be unusable on a 32-bit machine, right? How do other existing implementations handle this question? > > > I would like to answer a few unanswered questions. > > > > > What is bias? > > > > I' ve confirmed that the bias issue in mt_rand has already been fixed and > > is no longer a problem. > > > > This implementation copies most of it from mt_rand. Therefore, this > > problem does not exist. > > > > Therefore, an equivalent method to mt_getrandmax() is no longer provided. > Great! > > > > > uint64 & right-shift > > > > This is a specification of the RNG implementation. PHP does not have > > unsigned integers, but RNG handles unsigned integers (to be precise, even > > the sign bit is random). > > > > As mentioned above, PHP does not have unsigned integers, which means that > > using the results generated by RNGs may cause compatibility problems in > the > > future. To avoid this, a 1-bit right shift is always required (similar to > > mt_rand()). > Good to know, thanks. Regards, -- Guilliam Xavier
