Sorry. I couldn't make it in time for my lunch break. > How many bytes is `Source::next()` supposed to generate? Is it even intended to be called directly (vs `Randomizer::generateBytes(int $length)`)?
No, this is not intended to be used directly. In fact, as many people have pointed out, I think it would be more appropriate to merge it into a single class. As per the original implementation (ext/orng), I didn't think it needed to be separated originally. One of the comments on a previous RFC suggested that it would probably be better to separate them, and I took that into account. > About the `MT19937PHP` variant: The PHP manual for `mt_srand()` describes the `MT_RAND_PHP` mode like this: "Uses an *incorrect* Mersenne Twister implementation which was used as the default up till PHP 7.1.0. This mode is available for backward compatibility."; do we really need/want to port it to this new extension? This is for backward compatibility. If it is not needed, we would like to eliminate the implementation itself. Well, to keep the language core clean, maybe this should be provided by an appropriate external extension. - `PlatformProvided` feels like the "odd one out" here: its constructor [assuming same as previous RFC] doesn't take a seed, and it isn't serializable; I understand why, but the main point of the RFC is "RNG reproducibility", which this class cannot achieve. You're absolutely right. I would like to remove this. > Technical, about `Randomizer::generateFloat()`: Is the returned float guaranteed to be finite (i.e. never NAN nor +/-INF)? And may it ever return -0.0 (negative zero)? and even if not, is 0.0 twice as likely as other floats? Also, no `generateFloat(float $min, float $max)` variant like for `generateInt()`? As a matter of fact, we believe that the functions provided by this class are sufficient for `mt_rand()`, `shuffle()`, `str_shuffle()`, and `array_rand()` equivalents. At most, the equivalent of `random_byte()` might be provided. I'll reorganize these. Thanks for the detailed remarks. Based on these, I would like to clean up the RFC. Regards, Go Kudo 2021年5月19日(水) 18:46 Guilliam Xavier <guilliam.xav...@gmail.com>: > > > On Tue, May 18, 2021 at 6:19 PM Go Kudo <zeriyo...@gmail.com> wrote: > >> Hello internals. >> >> I have created a draft of the RFC. >> >> https://wiki.php.net/rfc/rng_extension >> > > Hello Go Kudo, > > First of all, thank you for the (re)work. > > I think the API looks better, but I still have some remarks/questions > (roughly in order of appearance, with some overlap): > > - How many bytes is `Source::next()` supposed to generate? Is it even > intended to be called directly (vs `Randomizer::generateBytes(int > $length)`)? > > - The stubs in "Proposal" should show `__construct()` signatures for the > classes implementing `Source`. > > - About the `MT19937PHP` variant: The PHP manual for `mt_srand()` > describes the `MT_RAND_PHP` mode like this: "Uses an *incorrect* Mersenne > Twister implementation which was used as the default up till PHP 7.1.0. > This mode is available for backward compatibility."; do we really need/want > to port it to this new extension? > > - I think you said in the past that `XorShift128Plus` should be the > preferred/default implementation, and `MT19937` is only provided for > compatibility/migration? But if so, that's not clear at all in the RFC. > > - `PlatformProvided` feels like the "odd one out" here: its constructor > [assuming same as previous RFC] doesn't take a seed, and it isn't > serializable; I understand why, but the main point of the RFC is "RNG > reproducibility", which this class cannot achieve. > Also, we already have `random_bytes()`/`random_int()`, and you're > proposing to change `shuffle()`/`str_shuffle()`/`array_rand()`'s internal > RNG from `mt_rand()`-like to `random_bytes()`-like, so what reason would > there be to use this class? only `generateFloat()`? but couldn't that be a > new global function `random_float()`? > All in all, wouldn't it be simpler to drop `PlatformProvided`, and have > all `Source` implementations take a seed on construction and be > serializable? > > - Technical, about `Randomizer::generateFloat()`: Is the returned float > guaranteed to be finite (i.e. never NAN nor +/-INF)? And may it ever return > -0.0 (negative zero)? and even if not, is 0.0 twice as likely as other > floats? > Also, no `generateFloat(float $min, float $max)` variant like for > `generateInt()`? > > - **About main usage of the API:** To get e.g. $n (say 3) pseudo-random > integers between $min (say 0) and $max (say 100) using the "best" > implementation (which we have to know/guess is XorShift128+, isn't it?), we > must: *first* choose a $seed (`time()`, maybe?), *then* do `$source = new > XorShift128Plus($seed);` *and* `$rng = new Randomizer($source);` [even if > $source (and $seed) could be inlined, that's still several "steps"], *then* > we can finally call `$rng->generateInt($min, $max);` $n times. Is that > correct? > If so, it may make sense from the implementer side, but from the user > side it doesn't look like the most friendly. > Also, that lets the possibility of constructing a second Randomizer > using the same $source (or `$rng->getSource()`), which could be undesirable > for reproducibility, isn't it? > Wouldn't it be better to "merge" Randomizer into the interface/classes? > Actually I see that's the case in your other project ext/orng ( > https://github.com/zeriyoshi/php-ext-orng/blob/master/rng/rnginterface.stub.php > and > https://github.com/zeriyoshi/php-ext-orng/blob/master/rng/xorshift128plus.stub.php > ), why the different approach for ext/rng? > Maybe even better (and already suggested), have Randomizer constructor > take an *optional* algorithm (string, default XorShift128Plus::class?) and > an *optional* seed (int, default time()?) and construct the source > internally (and don't expose it)? So we could do just `$rng = new > Randomizer();` to rely on the defaults (then call `$rng->generateInt($min, > $max);` $n times). > > - Finally, I think the vote should be split into 3 parts: the new classes, > the deprecations (which itself could be split between [mt_]srand and > [mt_]rand), and the change of internal RNG for > shuffle/str_shuffle/array_rand. > > Best regards, > > -- > Guilliam Xavier >
