Thanks Guilliam. > I'm not sure you really addressed https://externals.io/message/114561#114680 ?
I thought I had solved this problem by implementing the `RandomNumberGenerator` interface. Accepting strings and objects as arguments is certainly complicated, but I thought it met the necessary requirements. >From the aspect of static analysis, there seems to be no problem. Reverting to a full object-based approach would increase the cost of providing new RNGs from extensions. I think the string approach is superior in this respect. Nikita's `createDefault()` approach is certainly good, but I think it is still difficult for beginners to understand. I also thought about it for a while after that, and I think that the current implementation is the most appropriate for now. What do you think? > Couldn't the Random class simply implement `public function __clone(): void` (that would internally do like `$this->algo = clone $this->algo;`)? Implicitly cloning in areas where the user cannot interfere may cause problems when using objects that cannot be cloned. However, this may be overthinking it. The reason is that a `RandomNumberGenerator` that cannot be cloned should be implemented as algo in the first place. If there are no objections, I will change the implementation. > 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? I don't see any problem with the current implicit truncation behavior for this. Even if a user switches to a 64-bit environment, compatibility can be maintained by explicitly truncating the generated values. In addition, most of the other methods only use 32bit internally. If you are concerned about this, you should probably be concerned about the incompatibility with MT_RAND_PHP. That problem can also be compensated for with an extension that adds algo. Regards, Go Kudo 2021年6月9日(水) 19:07 Guilliam Xavier <guilliam.xav...@gmail.com>: > 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 >
